Re: [PATCH 22/30] fbdev/smscufx: Detect registered fb_info from refcount

Thomas Zimmermann <[email protected]> writes:

Hello Thomas,

> Hi Javier
>
> Am 08.06.23 um 00:22 schrieb Javier Martinez Canillas:
>> Thomas Zimmermann <[email protected]> writes:
>> 
>>> Detect registered instances of fb_info by reading the reference
>>> counter from struct fb_info.read. Avoids looking at the dev field
>>> and prepares fbdev for making struct fb_info.dev optional.
>>>
>>> Signed-off-by: Thomas Zimmermann <[email protected]>
>>> Cc: Steve Glendinning <[email protected]>
>>> ---
>>>   drivers/video/fbdev/smscufx.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/video/fbdev/smscufx.c b/drivers/video/fbdev/smscufx.c
>>> index 17cec62cc65d..adb2b1fe8383 100644
>>> --- a/drivers/video/fbdev/smscufx.c
>>> +++ b/drivers/video/fbdev/smscufx.c
>>> @@ -1496,7 +1496,7 @@ static int ufx_setup_modes(struct ufx_data *dev, struct fb_info *info,
>>>   	u8 *edid;
>>>   	int i, result = 0, tries = 3;
>>>   
>>> -	if (info->dev) /* only use mutex if info has been registered */
>>> +	if (refcount_read(&info->count)) /* only use mutex if info has been registered */
>> 
>> The struct fb_info .count refcount is protected by the registration_lock
>> mutex in register_framebuffer(). I think this operation isn't thread safe ?
>
> It's an atomic read.
>
> https://elixir.bootlin.com/linux/latest/source/include/linux/refcount.h#L147
>

Yes, is an atomic read but by reading [0] my understanding is that does
not provide memory ordering guarantees. Maybe I misunderstood though...

[0]: https://www.kernel.org/doc/html/latest/core-api/refcount-vs-atomic.html

> And that function is only being called from the USB probe callback 
> before registering the framebuffer. It's not clear to me how the value 
> could be anything but zero. I'd best leave it as is with the ref counter.
>

Yes, I'm OK with that and as mentioned I don't think that's less safe
than the previous info->dev check with regard to race conditions.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat