Re: [Linux-kernel-mentees] modprobing 'vidtv' doesn't really do anything

Re: [Linux-kernel-mentees] modprobing 'vidtv' doesn't really do anything

[Daniel W. S. Almeida]

With your current patch the probing code is actually called now, Mauro. Thanks!

As for the sparse errors you've pointed out earlier, I'm at a loss. Yes,
I have noticed them, but let's look at an example (they're all mostly
the same)

>SPARSE:drivers/media/test-drivers/vidtv/vidtv_psi.c
>drivers/media/test-drivers/vidtv/vidtv_psi.c:174:43: warning: Using plain
>integer as NULL pointer

This actually refers to this line:

>struct psi_write_args psi_args = {0}

Which seems to me like a valid way to zero-initialize a struct in C? 

Next up is...

>SPARSE:drivers/media/test-drivers/vidtv/vidtv_pes.c
>drivers/media/test-drivers/vidtv/vidtv_pes.c:80:54: warning: missing
>braces around initializer

I assume this refers to the following line, which is the same deal as above.

>struct vidtv_pes_optional_pts pts         = {0};


And then there's this, which is an actual mistake. I have mostly
eliminated these, but this guy has slipped by.

>SPARSE:drivers/media/test-drivers/vidtv/vidtv_s302m.c
>drivers/media/test-drivers/vidtv/vidtv_s302m.c:430:36: warning:
>incorrect type in assignment (different base types)


Just one more thing. This change on vidtv_bridge.c:

>vidtv_bridge_check_demod_lock(struct vidtv_dvb *dvb, u32 n)
>
>dvb->fe[n]->ops.read_status(dvb->fe[n], &status);
>
>-	return status == (FE_HAS_SIGNAL |
>-	FE_HAS_CARRIER |
>-	FE_HAS_VITERBI |
>-	FE_HAS_SYNC |
>-	FE_HAS_LOCK);
>+	status = FE_HAS_SIGNAL |
>+	FE_HAS_CARRIER |
>+	FE_HAS_VITERBI |
>+	FE_HAS_SYNC |
>+	FE_HAS_LOCK;
>+
>+	return status;
>}

I did not understand that. Why was the boolean expression replaced by an
assignment? This was so eventually we could drop packets or simulate
some sort of noise in the event that one of these flags was not set, as
we've discussed at some point.

Thanks,

- Daniel
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] modprobing 'vidtv' doesn't really do anything

[Mauro Carvalho Chehab]

Em Tue, 19 May 2020 04:13:07 -0300
"Daniel W. S. Almeida" <dwlsalmeida@gmail.com> escreveu:

> With your current patch the probing code is actually called now, Mauro. Thanks!
> 
> As for the sparse errors you've pointed out earlier, I'm at a loss. Yes,
> I have noticed them, but let's look at an example (they're all mostly
> the same)
> 
> >SPARSE:drivers/media/test-drivers/vidtv/vidtv_psi.c
> >drivers/media/test-drivers/vidtv/vidtv_psi.c:174:43: warning: Using plain
> >integer as NULL pointer  
> 
> This actually refers to this line:
> 
> >struct psi_write_args psi_args = {0}  
> 
> Which seems to me like a valid way to zero-initialize a struct in C? 

Actually, passing 0 is not the right thing there. The init code
should either be using gcc style:

	struct psi_write_args psi_args = {}  

or something where the first argument matches the first argument
of the struct, as, according with the C99 specs, doing something
like:

	struct foo {
		void *bar;
		...
	};

	struct psi_write_args psi_args = {NULL};

Would do is to use NULL to initialize "bar" variable. 

See comments about that at https://stackoverflow.com/questions/11152160/initializing-a-struct-to-0:

	"Reference C99 Standard 6.7.8.21:

	If there are fewer initializers in a brace-enclosed list than there are elements or members of an aggregate, or fewer characters in a string literal used to initialize an array of known size than there are elements in the array, the remainder of the aggregate shall be initialized implicitly the same as objects that have static storage duration."

The other values would be initialized with the "same as objects that
have static storage", e. g. with zero.

In other words, you need to check what's the first element of the
struct, when using C99 initialization. E. g, if the first element
is a pointer, the init should be:

	struct some_struct_that_starts_with_a_pointer = {NULL};

Tricky.

> 
> Next up is...
> 
> >SPARSE:drivers/media/test-drivers/vidtv/vidtv_pes.c
> >drivers/media/test-drivers/vidtv/vidtv_pes.c:80:54: warning: missing
> >braces around initializer  
> 
> I assume this refers to the following line, which is the same deal as above.
> 
> >struct vidtv_pes_optional_pts pts         = {0};  
> 
> 
> And then there's this, which is an actual mistake. I have mostly
> eliminated these, but this guy has slipped by.
> 
> >SPARSE:drivers/media/test-drivers/vidtv/vidtv_s302m.c
> >drivers/media/test-drivers/vidtv/vidtv_s302m.c:430:36: warning:
> >incorrect type in assignment (different base types)  
> 
> 
> Just one more thing. This change on vidtv_bridge.c:
> 
> >vidtv_bridge_check_demod_lock(struct vidtv_dvb *dvb, u32 n)
> >
> >dvb->fe[n]->ops.read_status(dvb->fe[n], &status);
> >
> >-	return status == (FE_HAS_SIGNAL |
> >-	FE_HAS_CARRIER |
> >-	FE_HAS_VITERBI |
> >-	FE_HAS_SYNC |
> >-	FE_HAS_LOCK);
> >+	status = FE_HAS_SIGNAL |
> >+	FE_HAS_CARRIER |
> >+	FE_HAS_VITERBI |
> >+	FE_HAS_SYNC |
> >+	FE_HAS_LOCK;
> >+
> >+	return status;
> >}  
> 
> I did not understand that. Why was the boolean expression replaced by an
> assignment? This was so eventually we could drop packets or simulate
> some sort of noise in the event that one of these flags was not set, as
> we've discussed at some point.

Ah, sorry, I remember gcc complained about that (can't remember why).

On a very quick look (it was 2am here when I looked at the code), it
sounded to me  that you wanted to assign status to some value and return
it.

Thanks,
Mauro
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] modprobing 'vidtv' doesn't really do anything

[Daniel W. S. Almeida]

Hi Mauro,

> Actually, passing 0 is not the right thing there. The init code
> should either be using gcc style:
> 
> 	struct psi_write_args psi_args = {}  
> 

I did not know that! Now fixed by replacing " = {0}" with " = {}"

As I said, the probing functions are actually called, but now I am
running into other issues..

I have sent a new revision (v6), in which I have squashed your patch
plus a few changes to the bridge driver.

In vidtv_bridge.c:266, dvb_module_probe always returns NULL. That is
because i2c_client_has_driver(client) fails, because client->dev.driver
is NULL.

I suspect this line is to blame, but I am not sure..
>	i2c_adapter->dev.parent = &dvb->pdev->dev;


Also, when this happens, some error handling code will run starting at
vidtv_bridge.c:383, but that will not remove the modules (i.e. lsmod
will still list the bridge and the demod). Is this expected? Should I
call vidtv_bridge_exit manually?

Also, should I worry about this?

>BUG: KASAN: user-memory-access in >do_init_module+0x1d/0x330
>Read of size 8 at addr 000000008322fe90 by task >modprobe/1290

I did not call this directly and I don't see a way to trace it back to
my code just by looking at "do_init_module+0x1d/0x330"

Thanks

- Daniel
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] modprobing 'vidtv' doesn't really do anything

[Daniel W. S. Almeida]

Yeah the Makefile was broken, although I corrected it in the last
revision, so this warning should be gone already?

I guess you're looking at v4, in which case I have sent in new code that
hopefully addresses the things you pointed out in the last code review :)

I will try what you said regarding the platform bus and report back.

- Daniel.
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] modprobing 'vidtv' doesn't really do anything

[Mauro Carvalho Chehab]

Em Sun, 17 May 2020 01:29:35 +0200
Mauro Carvalho Chehab <mchehab@kernel.org> escreveu:

> Em Sat, 16 May 2020 19:09:35 -0300
> "Daniel W. S. Almeida" <dwlsalmeida@gmail.com> escreveu:
> 
> > Hi Mauro!
> > 
> > I am trying to iron out the bugs in the virtual DVB driver I have been
> > working on for the past few months.
> > 
> > modprobing vidtv_bridge apparently works, i.e. 'vidtv_bridge' gets
> > listed in the output of 'lsmod' and there's a message on dmesg warning
> > against loading out of tree modules.
> 
> The warning is probably due to that:
> 
> WARNING: modpost: missing MODULE_LICENSE() in drivers/media/test-drivers/vidtv//vidtv_demod.o
> WARNING: modpost: missing MODULE_LICENSE() in drivers/media/test-drivers/vidtv//vidtv_bridge.o
> 
> You forgot to add MODULE_LICENSE() macro somewhere.
> 
> With is weird, as those are there:
> 
> 	drivers/media/test-drivers/vidtv/vidtv_bridge.c:MODULE_AUTHOR("Daniel W. S. Almeida");
> 	drivers/media/test-drivers/vidtv/vidtv_bridge.c:MODULE_LICENSE("GPL");
> 	drivers/media/test-drivers/vidtv/vidtv_bridge.c:MODULE_DEVICE_TABLE(i2c, vidtv_bridge_id_table);
> 
> (yet, a good practice nowadays is to place all those three at the end of
> a file - not at the beginning)
> 
> This is weird:
> 
> 	drivers/media/test-drivers/vidtv/vidtv_bridge.c:module_i2c_driver(vidtv_bridge_driver);
> 	drivers/media/test-drivers/vidtv/vidtv_demod.c:module_i2c_driver(vidtv_demod_i2c_driver);
> 	drivers/media/test-drivers/vidtv/vidtv_tuner.c:module_i2c_driver(vidtv_tuner_i2c_driver);
> 
> With the above, none of the three files will be initialized, as they
> would wait for some other driver to bind them on some I2C bus.
> 
> See, the way the Kernel works is that each bus has some sort
> of code that initializes the driver. For buses like PCI and USB,
> this happens when an USB ID or PCI ID matches their device tables.
> For normal[1] I2C devices, the driver which creates an I2C adapter
> should either manually bind new I2C devices or ask the I2C core
> to scan.
> 
> [1] ACPI devices using I2C buses use a different logic.
> 
> So, basically, the module_i2c_driver() tells the driver code
> that those devices will be available when some other driver
> would need to bind them. This is the right thing to do for the
> tuner and demod, but the bridge driver should do something else.
> 
> I would be expecting at the bridge driver something like:
> 
> 	static struct platform_driver vidtv_bridge_driver = {
> 		.probe		= vidtv_bridge_probe,
> 		.remove		= vidtv_bridge_remove,
> 		.driver		= {
> 			.name	= "vidtv-bridge",
> 		},
> 	};
> 	module_platform_driver(vidtv_bridge_driver);
> 
> Note: the other media virtual drivers don't use the 
> "module_platform_driver" macro. 
> 
> While I don't test this for a while, but looking at the code
> differences, they also declare a "platform_driver". So, they use 
> a more verbose syntax (see at the end). I suspect that this is
> needed for those devices to be probed unconditionally[2].
> 
> [1] a real platform driver is probed via DT or some other
> mechanism, like ACPI.
> 
> Thanks,
> Mauro
> 
> -
> 
> This is what vim2m.c does, for example:
> 
> 
> static struct platform_driver vim2m_pdrv = {
> 	.probe		= vim2m_probe,
> 	.remove		= vim2m_remove,
> 	.driver		= {
> 		.name	= MEM2MEM_NAME,
> 	},
> };
> 
> static void __exit vim2m_exit(void)
> {
> 	platform_driver_unregister(&vim2m_pdrv);
> 	platform_device_unregister(&vim2m_pdev);
> }
> 
> static int __init vim2m_init(void)
> {
> 	int ret;
> 
> 	ret = platform_device_register(&vim2m_pdev);
> 	if (ret)
> 		return ret;
> 
> 	ret = platform_driver_register(&vim2m_pdrv);
> 	if (ret)
> 		platform_device_unregister(&vim2m_pdev);
> 
> 	return ret;
> }
> 
> module_init(vim2m_init);
> module_exit(vim2m_exit);

There's also a problem at the Makefile. The way it was defined, the
vidtv-bridge driver won't be built.

The enclosed patch should fix the issues. The bridge driver doesn't 
build, though.

Thanks,
Mauro

---

diff --git a/drivers/media/test-drivers/vidtv/Makefile b/drivers/media/test-drivers/vidtv/Makefile
index a1d29001fffe..a56ad8bce0cb 100644
--- a/drivers/media/test-drivers/vidtv/Makefile
+++ b/drivers/media/test-drivers/vidtv/Makefile
@@ -1,7 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 
 vidtv_demod-objs := vidtv_common.o
-vidtv_bridge-objs := vidtv_common.o vidtv_ts.o vidtv_psi.o vidtv_pes.o \
-		     vidtv_s302m.o vidtv_channel.o vidtv_mux.o
+vidtv-objs := vidtv_common.o vidtv_ts.o vidtv_psi.o vidtv_pes.o \
+	      vidtv_s302m.o vidtv_channel.o vidtv_mux.o vidtv_bridge.o
 
 obj-$(CONFIG_DVB_VIDTV)	+= vidtv_tuner.o vidtv_demod.o vidtv_bridge.o
diff --git a/drivers/media/test-drivers/vidtv/vidtv_bridge.c b/drivers/media/test-drivers/vidtv/vidtv_bridge.c
index c9876372fdeb..762abf45dda0 100644
--- a/drivers/media/test-drivers/vidtv/vidtv_bridge.c
+++ b/drivers/media/test-drivers/vidtv/vidtv_bridge.c
@@ -20,9 +20,6 @@
 #define TUNER_DEFAULT_ADDR 0x68
 #define DEMOD_DEFAULT_ADDR 0x60
 
-MODULE_AUTHOR("Daniel W. S. Almeida");
-MODULE_LICENSE("GPL");
-
 static unsigned int drop_tslock_prob_on_low_snr;
 module_param(drop_tslock_prob_on_low_snr, uint, 0644);
 MODULE_PARM_DESC(drop_tslock_prob_on_low_snr,
@@ -422,21 +419,44 @@ static int vidtv_bridge_i2c_remove(struct i2c_client *client)
 	return 0;
 }
 
-static const struct i2c_device_id vidtv_bridge_id_table[] = {
-	{"vidtv_bridge", 0},
-	{}
+static struct platform_device vidtv_bridge_dev = {
+	.name		= "vidtv_bridge",
+	.dev.release	= vidtv_bridge_dev_release,
 };
 
-MODULE_DEVICE_TABLE(i2c, vidtv_bridge_id_table);
-
-static struct i2c_driver vidtv_bridge_driver = {
+static struct platform_driver vidtv_bridge_driver = {
 	.driver = {
 		.name                = "vidtv_bridge",
 		.suppress_bind_attrs = true,
 	},
 	.probe    = vidtv_bridge_i2c_probe,
 	.remove   = vidtv_bridge_i2c_remove,
-	.id_table = vidtv_bridge_id_table,
 };
 
-module_i2c_driver(vidtv_bridge_driver);
+static void __exit vidtv_bridge_exit(void)
+{
+	platform_driver_unregister(&vidtv_bridge_dev);
+	platform_device_unregister(&vidtv_bridge_driver);
+}
+
+static int __init vidtv_bridge_init(void)
+{
+	int ret;
+
+	ret = platform_device_register(&vidtv_bridge_dev);
+	if (ret)
+		return ret;
+
+	ret = platform_driver_register(&vidtv_bridge_driver);
+	if (ret)
+		platform_device_unregister(&vidtv_bridge_pdev);
+
+	return ret;
+}
+
+module_init(vidtv_bridge_init);
+module_exit(vidtv_bridge_exit);
+
+MODULE_DESCRIPTION("Virtual Digital TV Test Driver");
+MODULE_AUTHOR("Daniel W. S. Almeida");
+MODULE_LICENSE("GPL");
diff --git a/drivers/media/test-drivers/vidtv/vidtv_demod.c b/drivers/media/test-drivers/vidtv/vidtv_demod.c
index 15436e565a7b..01cd4a93b0ec 100644
--- a/drivers/media/test-drivers/vidtv/vidtv_demod.c
+++ b/drivers/media/test-drivers/vidtv/vidtv_demod.c
@@ -21,10 +21,6 @@
 #include "vidtv_demod.h"
 #include "vidtv_config.h"
 
-MODULE_DESCRIPTION("Virtual DVB Demodulator Driver");
-MODULE_AUTHOR("Daniel W. S. Almeida");
-MODULE_LICENSE("GPL");
-
 struct vidtv_demod_cnr_to_qual_s vidtv_demod_c_cnr_2_qual[] = {
 	/* from libdvbv5 source code, in milli db */
 	{ QAM_256, FEC_NONE,  34000, 38000},
@@ -492,3 +488,7 @@ static struct i2c_driver vidtv_demod_i2c_driver = {
 };
 
 module_i2c_driver(vidtv_demod_i2c_driver);
+
+MODULE_DESCRIPTION("Virtual DVB Demodulator Driver");
+MODULE_AUTHOR("Daniel W. S. Almeida");
+MODULE_LICENSE("GPL");

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] modprobing 'vidtv' doesn't really do anything

[Mauro Carvalho Chehab]

Em Sat, 16 May 2020 19:09:35 -0300
"Daniel W. S. Almeida" <dwlsalmeida@gmail.com> escreveu:

> Hi Mauro!
> 
> I am trying to iron out the bugs in the virtual DVB driver I have been
> working on for the past few months.
> 
> modprobing vidtv_bridge apparently works, i.e. 'vidtv_bridge' gets
> listed in the output of 'lsmod' and there's a message on dmesg warning
> against loading out of tree modules.

The warning is probably due to that:

WARNING: modpost: missing MODULE_LICENSE() in drivers/media/test-drivers/vidtv//vidtv_demod.o
WARNING: modpost: missing MODULE_LICENSE() in drivers/media/test-drivers/vidtv//vidtv_bridge.o

You forgot to add MODULE_LICENSE() macro somewhere.

With is weird, as those are there:

	drivers/media/test-drivers/vidtv/vidtv_bridge.c:MODULE_AUTHOR("Daniel W. S. Almeida");
	drivers/media/test-drivers/vidtv/vidtv_bridge.c:MODULE_LICENSE("GPL");
	drivers/media/test-drivers/vidtv/vidtv_bridge.c:MODULE_DEVICE_TABLE(i2c, vidtv_bridge_id_table);

(yet, a good practice nowadays is to place all those three at the end of
a file - not at the beginning)

This is weird:

	drivers/media/test-drivers/vidtv/vidtv_bridge.c:module_i2c_driver(vidtv_bridge_driver);
	drivers/media/test-drivers/vidtv/vidtv_demod.c:module_i2c_driver(vidtv_demod_i2c_driver);
	drivers/media/test-drivers/vidtv/vidtv_tuner.c:module_i2c_driver(vidtv_tuner_i2c_driver);

With the above, none of the three files will be initialized, as they
would wait for some other driver to bind them on some I2C bus.

See, the way the Kernel works is that each bus has some sort
of code that initializes the driver. For buses like PCI and USB,
this happens when an USB ID or PCI ID matches their device tables.
For normal[1] I2C devices, the driver which creates an I2C adapter
should either manually bind new I2C devices or ask the I2C core
to scan.

[1] ACPI devices using I2C buses use a different logic.

So, basically, the module_i2c_driver() tells the driver code
that those devices will be available when some other driver
would need to bind them. This is the right thing to do for the
tuner and demod, but the bridge driver should do something else.

I would be expecting at the bridge driver something like:

	static struct platform_driver vidtv_bridge_driver = {
		.probe		= vidtv_bridge_probe,
		.remove		= vidtv_bridge_remove,
		.driver		= {
			.name	= "vidtv-bridge",
		},
	};
	module_platform_driver(vidtv_bridge_driver);

Note: the other media virtual drivers don't use the 
"module_platform_driver" macro. 

While I don't test this for a while, but looking at the code
differences, they also declare a "platform_driver". So, they use 
a more verbose syntax (see at the end). I suspect that this is
needed for those devices to be probed unconditionally[2].

[1] a real platform driver is probed via DT or some other
mechanism, like ACPI.

Thanks,
Mauro

-

This is what vim2m.c does, for example:


static struct platform_driver vim2m_pdrv = {
	.probe		= vim2m_probe,
	.remove		= vim2m_remove,
	.driver		= {
		.name	= MEM2MEM_NAME,
	},
};

static void __exit vim2m_exit(void)
{
	platform_driver_unregister(&vim2m_pdrv);
	platform_device_unregister(&vim2m_pdev);
}

static int __init vim2m_init(void)
{
	int ret;

	ret = platform_device_register(&vim2m_pdev);
	if (ret)
		return ret;

	ret = platform_driver_register(&vim2m_pdrv);
	if (ret)
		platform_device_unregister(&vim2m_pdev);

	return ret;
}

module_init(vim2m_init);
module_exit(vim2m_exit);
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

[Linux-kernel-mentees] modprobing 'vidtv' doesn't really do anything

[Daniel W. S. Almeida]

Hi Mauro!

I am trying to iron out the bugs in the virtual DVB driver I have been
working on for the past few months.

modprobing vidtv_bridge apparently works, i.e. 'vidtv_bridge' gets
listed in the output of 'lsmod' and there's a message on dmesg warning
against loading out of tree modules.

It does not seem like the probe function actually runs, though. I have
added a printk call in vidtv_bridge_i2c_probe() but it does not output
anything to dmesg.

Also, I am using QEMU + KGDB and the debugger does not break into this
function (or into any other function in vidtv_bridg.c) at all, although
it can actually get the addresses for them, i.e.

4       breakpoint     keep y   0xffffffffc0000025 in
vidtv_bridge_i2c_probe at drivers/media/test-drivers/vidtv/vidtv_bridg.c:373
5       breakpoint     keep y   0xffffffffc0000523 in
vidtv_bridge_i2c_remove at drivers/media/test-drivers/vidtv/vidtv_bridg.c:416

It can't find these, though:
2       breakpoint     keep y   <PENDING>          drivers/media/test-drivers/vidtv/vidtv_bridg.c:vidtv_bridge_driver_exit
3       breakpoint     keep y   <PENDING>          drivers/media/test-drivers/vidtv/vidtv_bridg.c:vidtv_bridge_driver_init


>The best is to start using dvbv5 tools inside v4l-utils.
>The first step to check if the demod code was properly loaded is to run:
>$ dvb-fe-tool 

I tried this, but to no avail.
>WARNING device dvb0.frontend0 not found

I am stuck, can I get some help with this issue?

Thanks!

-Daniel
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees