[questions] dmesg: Non-NULL drvdata on register

[questions] dmesg: Non-NULL drvdata on register

[Alexey Klimov]

Hello, 

Not so many time ago i noticed such line in dmesg:

radio-mr800 2-1:1.0: Non-NULL drvdata on register

Quick review showed that it appears in usb_amradio_probe fucntions. Then
i found such code in v4l2_device_register() function (v4l2-device.c
file):

/* Set name to driver name + device name if it is empty. */
        if (!v4l2_dev->name[0])
                snprintf(v4l2_dev->name, sizeof(v4l2_dev->name), "%s %
s",
                        dev->driver->name, dev_name(dev));
        if (dev_get_drvdata(dev))
                v4l2_warn(v4l2_dev, "Non-NULL drvdata on register\n");
        dev_set_drvdata(dev, v4l2_dev);
        return 0;

The questions is - should i deal with this warning in dmesg? Probably
the order of callbacks in radio-mr800 probe function is incorrect.

The second questions - should i make atomic_t users counter instead of
int users counter? Then i can use atomic_inc(), atomic_dec(),
atomic_set(). It helps me to remove lock/unlock_kernel() functions. 

-- 
Best regards, Klimov Alexey

Re: [questions] dmesg: Non-NULL drvdata on register

[Hans Verkuil]

> Hello,
>
> Not so many time ago i noticed such line in dmesg:
>
> radio-mr800 2-1:1.0: Non-NULL drvdata on register
>
> Quick review showed that it appears in usb_amradio_probe fucntions. Then
> i found such code in v4l2_device_register() function (v4l2-device.c
> file):
>
> /* Set name to driver name + device name if it is empty. */
>         if (!v4l2_dev->name[0])
>                 snprintf(v4l2_dev->name, sizeof(v4l2_dev->name), "%s %
> s",
>                         dev->driver->name, dev_name(dev));
>         if (dev_get_drvdata(dev))
>                 v4l2_warn(v4l2_dev, "Non-NULL drvdata on register\n");
>         dev_set_drvdata(dev, v4l2_dev);
>         return 0;
>
> The questions is - should i deal with this warning in dmesg? Probably
> the order of callbacks in radio-mr800 probe function is incorrect.

I (or you :-) should look into this: I think the usb subsystem is calling
dev_set_drvdata as well, so we could have a clash here.

> The second questions - should i make atomic_t users counter instead of
> int users counter? Then i can use atomic_inc(), atomic_dec(),
> atomic_set(). It helps me to remove lock/unlock_kernel() functions.

'users' can go away completely: if you grep for it, then you'll see that
it is only set, but never used.

I think I've commented on the kernel lock before: I think it is bogus
here. And that the amradio_set_mute handling is wrong as well: you open
the radio device twice, then close one file descriptor, and suddenly the
audio will be muted, even though there still is a file descriptor open.

Regards,

        Hans

>
> --
> Best regards, Klimov Alexey
>
>


-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: [questions] dmesg: Non-NULL drvdata on register

[Hans Verkuil]

> Hello,
>
> Not so many time ago i noticed such line in dmesg:
>
> radio-mr800 2-1:1.0: Non-NULL drvdata on register
>
> Quick review showed that it appears in usb_amradio_probe fucntions. Then
> i found such code in v4l2_device_register() function (v4l2-device.c
> file):
>
> /* Set name to driver name + device name if it is empty. */
>         if (!v4l2_dev->name[0])
>                 snprintf(v4l2_dev->name, sizeof(v4l2_dev->name), "%s %
> s",
>                         dev->driver->name, dev_name(dev));
>         if (dev_get_drvdata(dev))
>                 v4l2_warn(v4l2_dev, "Non-NULL drvdata on register\n");
>         dev_set_drvdata(dev, v4l2_dev);
>         return 0;
>
> The questions is - should i deal with this warning in dmesg? Probably
> the order of callbacks in radio-mr800 probe function is incorrect.

I (or you :-) should look into this: I think the usb subsystem is calling
dev_set_drvdata as well, so we could have a clash here.

> The second questions - should i make atomic_t users counter instead of
> int users counter? Then i can use atomic_inc(), atomic_dec(),
> atomic_set(). It helps me to remove lock/unlock_kernel() functions.

'users' can go away completely: if you grep for it, then you'll see that
it is only set, but never used.

I think I've commented on the kernel lock before: I think it is bogus
here. And that the amradio_set_mute handling is wrong as well: you open
the radio device twice, then close one file descriptor, and suddenly the
audio will be muted, even though there still is a file descriptor open.

Regards,

        Hans

>
> --
> Best regards, Klimov Alexey
>
>


-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: [questions] dmesg: Non-NULL drvdata on register

[Alexey Klimov]

On Mon, May 4, 2009 at 5:03 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
>> Hello,
>>
>> Not so many time ago i noticed such line in dmesg:
>>
>> radio-mr800 2-1:1.0: Non-NULL drvdata on register
>>
>> Quick review showed that it appears in usb_amradio_probe fucntions. Then
>> i found such code in v4l2_device_register() function (v4l2-device.c
>> file):
>>
>> /* Set name to driver name + device name if it is empty. */
>>         if (!v4l2_dev->name[0])
>>                 snprintf(v4l2_dev->name, sizeof(v4l2_dev->name), "%s %
>> s",
>>                         dev->driver->name, dev_name(dev));
>>         if (dev_get_drvdata(dev))
>>                 v4l2_warn(v4l2_dev, "Non-NULL drvdata on register\n");
>>         dev_set_drvdata(dev, v4l2_dev);
>>         return 0;
>>
>> The questions is - should i deal with this warning in dmesg? Probably
>> the order of callbacks in radio-mr800 probe function is incorrect.
>
> I (or you :-) should look into this: I think the usb subsystem is calling
> dev_set_drvdata as well, so we could have a clash here.

I can if i'll find free time for this :)

>> The second questions - should i make atomic_t users counter instead of
>> int users counter? Then i can use atomic_inc(), atomic_dec(),
>> atomic_set(). It helps me to remove lock/unlock_kernel() functions.
>
> 'users' can go away completely: if you grep for it, then you'll see that
> it is only set, but never used.
>
> I think I've commented on the kernel lock before: I think it is bogus
> here. And that the amradio_set_mute handling is wrong as well: you open
> the radio device twice, then close one file descriptor, and suddenly the
> audio will be muted, even though there still is a file descriptor open.
>
> Regards,
>
>        Hans

Well, looks like it's possible to null usb_amradio_open, right?
The only reason why users counter here is suspend/resume handling.
The idea was to stop radio in suspend procedure if there users>0 and
start radio on resume if users>0. If there are no users we can do
nothing on suspend/resume. The decision was based on radio->users
counter, and i see that i can do this using radio->muted.
This idea was not implemented yet. May i ask your comments here, Hans?

-- 
Best regards, Klimov Alexey

Re: [questions] dmesg: Non-NULL drvdata on register

[Janne Grunau]

On Mon, May 04, 2009 at 03:03:40PM +0200, Hans Verkuil wrote:
> 
> > Not so many time ago i noticed such line in dmesg:
> >
> > radio-mr800 2-1:1.0: Non-NULL drvdata on register
> >
> > Quick review showed that it appears in usb_amradio_probe fucntions. Then
> > i found such code in v4l2_device_register() function (v4l2-device.c
> > file):
> >
> > /* Set name to driver name + device name if it is empty. */
> >         if (!v4l2_dev->name[0])
> >                 snprintf(v4l2_dev->name, sizeof(v4l2_dev->name), "%s %
> > s",
> >                         dev->driver->name, dev_name(dev));
> >         if (dev_get_drvdata(dev))
> >                 v4l2_warn(v4l2_dev, "Non-NULL drvdata on register\n");
> >         dev_set_drvdata(dev, v4l2_dev);
> >         return 0;
> >
> > The questions is - should i deal with this warning in dmesg? Probably
> > the order of callbacks in radio-mr800 probe function is incorrect.
> 
> I (or you :-) should look into this: I think the usb subsystem is calling
> dev_set_drvdata as well, so we could have a clash here.

I don't think so. But probably all USB drivers call usb_set_intfdata
(just a wrapper around dev_set_drvdata). Most (media) drivers use it to
get driver struct back on a disconnect.

My change to use usb interface's struct device for v4l2_device_register
caused an oops on disconnect for em28xx based devices. Other drivers
(hdpvr, au0828) fortunately call usb_set_intfdata after calling
v4l2_device_register.

I'm not sure how to resolve this. USB drivers need for the disconnect a
way of of getting their private driver struct. We could add a data field
to v4l2_device. A couple of drivers seems to be already based on the
assumption that dev_get_drvdata(parent.dev) returns a v4l2_device.

Janne