diff mbox

zr36067 no longer loads automatically (regression)

Message ID 20090108175627.0ebd9f36@pedra.chehab.org (mailing list archive)
State Accepted
Headers show

Commit Message

Mauro Carvalho Chehab Jan. 8, 2009, 7:56 p.m. UTC 
> Hi Mauro,
> 
> This commit of yours:
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=60b4bde48b36c0315ef41fd38c339b9c7e68c46f
> causes the zr36067 driver to no longer load automatically on systems
> with a Zoran-based adapter. The comment says that zr36067_pci_tbl was
> unused but this isn't true when the zr36067 driver is built as a module
> (which is almost always the case.) zr36067_pci_tbl is used to generate
> the modprobe alias which lets udev load the zr36067 driver.
Hmm... I've forgot about udev. Anyway, just reverting it won't work fine, since, when compiled in-kernel, it generates warnings and adds a code that won't be used anywere.

It seems that the proper fix is the enclosed patch. Could you test it please?

---
zoran: Re-adds udev entry removed by changeset 60b4bde48b36c0315ef41fd38c339b9c7e68c46f

Changeset 60b4bde48b36c0315ef41fd38c339b9c7e68c46f removed an unused struct on
zoran driver, when compiled with "Y". However, as pointed by Jean Delvare <khali@linux-fr.org>,
this is neeeded when the driver is compiled as a module, since udev relies on it to 
auto-load the module.

Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>

Comments

Trent Piepho Jan. 8, 2009, 9:20 p.m. UTC | #1 
On Thu, 8 Jan 2009, Mauro Carvalho Chehab wrote:
> > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=60b4bde48b36c0315ef41fd38c339b9c7e68c46f
> > causes the zr36067 driver to no longer load automatically on systems
> > with a Zoran-based adapter. The comment says that zr36067_pci_tbl was
> > unused but this isn't true when the zr36067 driver is built as a module
> > (which is almost always the case.) zr36067_pci_tbl is used to generate
> > the modprobe alias which lets udev load the zr36067 driver.
> Hmm... I've forgot about udev. Anyway, just reverting it won't work fine, since, when compiled in-kernel, it generates warnings and adds a code that won't be used anywere.
>
> It seems that the proper fix is the enclosed patch. Could you test it please?

It doesn't seem like any other driver needs to protect the module device
table with an ifdef.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mauro Carvalho Chehab Jan. 8, 2009, 9:39 p.m. UTC | #2 
On Thu, 8 Jan 2009 13:20:19 -0800 (PST)
Trent Piepho <xyzzy@speakeasy.org> wrote:

> On Thu, 8 Jan 2009, Mauro Carvalho Chehab wrote:
> > > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=60b4bde48b36c0315ef41fd38c339b9c7e68c46f
> > > causes the zr36067 driver to no longer load automatically on systems
> > > with a Zoran-based adapter. The comment says that zr36067_pci_tbl was
> > > unused but this isn't true when the zr36067 driver is built as a module
> > > (which is almost always the case.) zr36067_pci_tbl is used to generate
> > > the modprobe alias which lets udev load the zr36067 driver.
> > Hmm... I've forgot about udev. Anyway, just reverting it won't work fine, since, when compiled in-kernel, it generates warnings and adds a code that won't be used anywere.
> >
> > It seems that the proper fix is the enclosed patch. Could you test it please?
> 
> It doesn't seem like any other driver needs to protect the module device
> table with an ifdef.

The other drivers use the PCI table for probing the device, using this construction:

static struct pci_device_id bttv_pci_tbl[] = {
        {PCI_VENDOR_ID_BROOKTREE, PCI_DEVICE_ID_BT848, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0},
        {0,}
};

MODULE_DEVICE_TABLE(pci, bttv_pci_tbl);

static struct pci_driver bttv_pci_driver = {
        .name     = "bttv",
        .id_table = bttv_pci_tbl,
        .probe    = bttv_probe,
        .remove   = __devexit_p(bttv_remove),
};

static int __init bttv_init_module(void)
{
	...
	ret = pci_register_driver(&bttv_pci_driver);
	...
}

With this, the PCI bus will take a look at the PCI ID table run the .probe
callback if a board with the given PCI ID's is detected.

However, Zoran driver doesn't rely on pci_register_driver(). Instead, it uses a
while() loop to probe for Zoran devices:

static int __devinit
find_zr36057 (void)
{
	...

        zoran_num = 0;
        while (zoran_num < BUZ_MAX &&
               (dev = pci_get_device(PCI_VENDOR_ID_ZORAN, PCI_DEVICE_ID_ZORAN_36057, dev)) != NULL) {
	...
}



Cheers,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jean Delvare Jan. 8, 2009, 9:45 p.m. UTC | #3 
Hi Mauro,

On Thu, 8 Jan 2009 17:56:27 -0200, Mauro Carvalho Chehab wrote:
> > Hi Mauro,
> > 
> > This commit of yours:
> > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=60b4bde48b36c0315ef41fd38c339b9c7e68c46f
> > causes the zr36067 driver to no longer load automatically on systems
> > with a Zoran-based adapter. The comment says that zr36067_pci_tbl was
> > unused but this isn't true when the zr36067 driver is built as a module
> > (which is almost always the case.) zr36067_pci_tbl is used to generate
> > the modprobe alias which lets udev load the zr36067 driver.
> Hmm... I've forgot about udev. Anyway, just reverting it won't work fine,
> since, when compiled in-kernel, it generates warnings and adds a code that
> won't be used anywere.

That's right, although I don't expect it to be a problem in practice:
who really builds media drivers into their kernels?

> 
> It seems that the proper fix is the enclosed patch. Could you test it please?
> 
> ---
> zoran: Re-adds udev entry removed by changeset 60b4bde48b36c0315ef41fd38c339b9c7e68c46f
> 
> Changeset 60b4bde48b36c0315ef41fd38c339b9c7e68c46f removed an unused struct on
> zoran driver, when compiled with "Y". However, as pointed by Jean Delvare <khali@linux-fr.org>,
> this is neeeded when the driver is compiled as a module, since udev relies on it to 
> auto-load the module.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
> 
> diff --git a/drivers/media/video/zoran/zoran_card.c b/drivers/media/video/zoran/zoran_card.c
> index 05f3919..0a50700 100644
> --- a/drivers/media/video/zoran/zoran_card.c
> +++ b/drivers/media/video/zoran/zoran_card.c
> @@ -153,6 +153,14 @@ MODULE_DESCRIPTION("Zoran-36057/36067 JPEG codec driver");
>  MODULE_AUTHOR("Serguei Miridonov");
>  MODULE_LICENSE("GPL");
>  
> +#if (defined(CONFIG_USB_ZR364XX_MODULE) && defined(MODULE))

Huh? I guess you really mean: CONFIG_VIDEO_ZORAN_MODULE?

I don't get the point of checking for both CONFIG_VIDEO_ZORAN_MODULE
and MODULE, checking for just one of them should be enough, right?

> +static struct pci_device_id zr36067_pci_tbl[] = {
> +	{PCI_VENDOR_ID_ZORAN, PCI_DEVICE_ID_ZORAN_36057,
> +	 PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0},
> +	{0}
> +};
> +MODULE_DEVICE_TABLE(pci, zr36067_pci_tbl);
> +#endif
>  
>  int zoran_num;			/* number of Buzs in use */
>  struct zoran *zoran[BUZ_MAX];
> 
>
Mauro Carvalho Chehab Jan. 8, 2009, 9:54 p.m. UTC | #4 
On Thu, 8 Jan 2009 22:45:20 +0100
Jean Delvare <khali@linux-fr.org> wrote:

> Hi Mauro,
> 
> On Thu, 8 Jan 2009 17:56:27 -0200, Mauro Carvalho Chehab wrote:
> > > Hi Mauro,
> > > 
> > > This commit of yours:
> > > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=60b4bde48b36c0315ef41fd38c339b9c7e68c46f
> > > causes the zr36067 driver to no longer load automatically on systems
> > > with a Zoran-based adapter. The comment says that zr36067_pci_tbl was
> > > unused but this isn't true when the zr36067 driver is built as a module
> > > (which is almost always the case.) zr36067_pci_tbl is used to generate
> > > the modprobe alias which lets udev load the zr36067 driver.
> > Hmm... I've forgot about udev. Anyway, just reverting it won't work fine,
> > since, when compiled in-kernel, it generates warnings and adds a code that
> > won't be used anywere.
> 
> That's right, although I don't expect it to be a problem in practice:
> who really builds media drivers into their kernels?

I do, before generating the patches upstream ;) This warning message were really bugging me, since I'm seeing it on every push requests during a long time.

> > 
> > It seems that the proper fix is the enclosed patch. Could you test it please?
> > 
> > ---
> > zoran: Re-adds udev entry removed by changeset 60b4bde48b36c0315ef41fd38c339b9c7e68c46f
> > 
> > Changeset 60b4bde48b36c0315ef41fd38c339b9c7e68c46f removed an unused struct on
> > zoran driver, when compiled with "Y". However, as pointed by Jean Delvare <khali@linux-fr.org>,
> > this is neeeded when the driver is compiled as a module, since udev relies on it to 
> > auto-load the module.
> > 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
> > 
> > diff --git a/drivers/media/video/zoran/zoran_card.c b/drivers/media/video/zoran/zoran_card.c
> > index 05f3919..0a50700 100644
> > --- a/drivers/media/video/zoran/zoran_card.c
> > +++ b/drivers/media/video/zoran/zoran_card.c
> > @@ -153,6 +153,14 @@ MODULE_DESCRIPTION("Zoran-36057/36067 JPEG codec driver");
> >  MODULE_AUTHOR("Serguei Miridonov");
> >  MODULE_LICENSE("GPL");
> >  
> > +#if (defined(CONFIG_USB_ZR364XX_MODULE) && defined(MODULE))
> 
> Huh? I guess you really mean: CONFIG_VIDEO_ZORAN_MODULE?

Yes.

> I don't get the point of checking for both CONFIG_VIDEO_ZORAN_MODULE
> and MODULE, checking for just one of them should be enough, right?

On all places were we test for foo_MODULE, we test also for MODULE. This were due
to some trouble, but I can't remember what were was the issue...
Trent Piepho Jan. 9, 2009, 6:16 a.m. UTC | #5 
On Thu, 8 Jan 2009, Jean Delvare wrote:
> > +#if (defined(CONFIG_USB_ZR364XX_MODULE) && defined(MODULE))
>
> Huh? I guess you really mean: CONFIG_VIDEO_ZORAN_MODULE?
>
> I don't get the point of checking for both CONFIG_VIDEO_ZORAN_MODULE
> and MODULE, checking for just one of them should be enough, right?

Only one should be necessary.  Sometimes one writes something like this:

#if defined(CONFIG_FOO) || (defined(CONFIG_FOO_MODULE) && defined(MODULE))
/* use symbol that's provided by FOO */
#endif

But that's for using a symbol provided by another module without depending
on that module.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/media/video/zoran/zoran_card.c b/drivers/media/video/zoran/zoran_card.c
index 05f3919..0a50700 100644
--- a/drivers/media/video/zoran/zoran_card.c
+++ b/drivers/media/video/zoran/zoran_card.c
@@ -153,6 +153,14 @@  MODULE_DESCRIPTION("Zoran-36057/36067 JPEG codec driver");
 MODULE_AUTHOR("Serguei Miridonov");
 MODULE_LICENSE("GPL");
 
+#if (defined(CONFIG_USB_ZR364XX_MODULE) && defined(MODULE))
+static struct pci_device_id zr36067_pci_tbl[] = {
+	{PCI_VENDOR_ID_ZORAN, PCI_DEVICE_ID_ZORAN_36057,
+	 PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0},
+	{0}
+};
+MODULE_DEVICE_TABLE(pci, zr36067_pci_tbl);
+#endif
 
 int zoran_num;			/* number of Buzs in use */
 struct zoran *zoran[BUZ_MAX];