Thomas Zimmermann <tzimmermann@xxxxxxx> writes: Hello Thomas, > Hi Javier > > Am 08.06.23 um 00:22 schrieb Javier Martinez Canillas: >> Thomas Zimmermann <tzimmermann@xxxxxxx> 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 <tzimmermann@xxxxxxx> >>> Cc: Steve Glendinning <steve.glendinning@xxxxxxxxxxx> >>> --- >>> 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
