[lkml]   [2019]   [Jun]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: A potential broken at platform driver?

Hi Greg,

On 6/4/19 12:03 PM, Greg KH wrote:
> On Tue, Jun 04, 2019 at 11:13:02AM -0500, Richard Gong wrote:
>> Hi Greg,
>> On 6/4/19 9:28 AM, Greg KH wrote:
>>> On Tue, Jun 04, 2019 at 12:33:03PM +0200, Romain Izard wrote:
>>>> On Mon, Jun 03, 2019 at 08:02:55PM +0200, Greg KH wrote:
>>>>>> @@ -394,7 +432,7 @@ static struct platform_driver stratix10_rsu_driver = {
>>>>>> .remove = stratix10_rsu_remove,
>>>>>> .driver = {
>>>>>> .name = "stratix10-rsu",
>>>>>> - .groups = rsu_groups,
>>>>>> +// .groups = rsu_groups,
>>>>> Are you sure this is the correct pointer? I think that might be
>>>>> pointing to the driver's attributes, not the device's attributes.
>>>>> If platform drivers do not have a way to register groups properly, then
>>>>> that really needs to be fixed, as trying to register it by yourself as
>>>>> you are doing, is ripe for racing with userspace.
>>>> This is a very common issue with platform drivers, and it seems to me that
>>>> it is not possible to add device attributes when binding a device to a
>>>> driver without entering the race condition.
>>>> My understanding is the following one:
>>>> The root cause is that the device has already been created and reported
>>>> to the userspace with a KOBJ_ADD uevent before the device and the driver
>>>> are bound together. On receiving this event, userspace will react, and
>>>> it will try to read the device's attributes. In parallel the kernel will
>>>> try to find a matching driver. If a driver is found, the kernel will
>>>> call the probe function from the driver with the device as a parameter,
>>>> and if successful a KOBJ_BIND uevent will be sent to userspace, but this
>>>> is a recent addition.
>>>> Unfortunately, not all created devices will be bound to a driver, and the
>>>> existing udev code relies on KOBJ_ADD uevents rather than KOBJ_BIND uevents.
>>>> If new per-device attributes have been added to the device during the
>>>> binding stage userspace may or may not see them, depending on when userspace
>>>> tries to read the device's attributes.
>>>> I have this possible workaround, but I do not know if it is a good solution:
>>>> When binding the device and the driver together, create a new device as a
>>>> child to the current device, and fill its "groups" member to point to the
>>>> per-device attributes' group. As the device will be created with all the
>>>> attributes, it will not be affected by the race issues. The functions
>>>> handling the attributes will need to be modified to use the parents of their
>>>> "device" parameter, instead of the device itself. Additionnaly, the sysfs
>>>> location of the attributes will be different, as the child device will show
>>>> up in the sysfs path. But for a newly introduced device this will not be
>>>> a problem.
>>>> Is this a good compromise ?
>>> Not really. You just want the attributes on the platform device itself.
>>> Given the horrible hack that platform devices are today, what's one more
>>> hack!
>>> Here's a patch below of what should probably be done here. Richard, can
>>> you change your code to use the new dev_groups pointer in the struct
>>> platform_driver and this patch and let me know if that works or not?
>>> Note, I've only compiled this code, not tested it...
>> Your patch works.
>> Many thanks for your help!
> Nice!
> I guess I need to turn it into a real patch now. Let me do that tonight
> and see if I can convert some existing drivers to use it as well...

Sorry for asking.

I haven't seen your patch, did you release that?


> thanks,
> greg k-h

 \ /
  Last update: 2019-06-11 15:58    [W:0.161 / U:18.496 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site