* Re: [PATCH 4/5] auxdisplay: linedisp: support attribute attachment to auxdisplay devices
@ 2025-09-02 10:38 kernel test robot
0 siblings, 0 replies; 6+ messages in thread
From: kernel test robot @ 2025-09-02 10:38 UTC (permalink / raw)
To: oe-kbuild; +Cc: lkp, Dan Carpenter
BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <20250901020033.60196-5-jefflessard3@gmail.com>
References: <20250901020033.60196-5-jefflessard3@gmail.com>
TO: "Jean-François Lessard" <jefflessard3@gmail.com>
TO: Andy Shevchenko <andy@kernel.org>
TO: Geert Uytterhoeven <geert@linux-m68k.org>
CC: linux-kernel@vger.kernel.org
Hi Jean-François,
kernel test robot noticed the following build warnings:
[auto build test WARNING on linus/master]
[also build test WARNING on v6.17-rc4 next-20250902]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Jean-Fran-ois-Lessard/auxdisplay-linedisp-encapsulate-container_of-usage-within-to_linedisp/20250901-100347
base: linus/master
patch link: https://lore.kernel.org/r/20250901020033.60196-5-jefflessard3%40gmail.com
patch subject: [PATCH 4/5] auxdisplay: linedisp: support attribute attachment to auxdisplay devices
:::::: branch date: 32 hours ago
:::::: commit date: 32 hours ago
config: x86_64-randconfig-161-20250902 (https://download.01.org/0day-ci/archive/20250902/202509021722.wnbbAMB8-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14+deb12u1) 12.2.0
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Closes: https://lore.kernel.org/r/202509021722.wnbbAMB8-lkp@intel.com/
smatch warnings:
drivers/auxdisplay/line-display.c:81 delete_attachment() warn: iterator used outside loop: 'attachment'
drivers/auxdisplay/line-display.c:102 to_linedisp() warn: iterator used outside loop: 'attachment'
vim +/attachment +81 drivers/auxdisplay/line-display.c
5b645f9dfbbe40 Jean-François Lessard 2025-08-31 64
5b645f9dfbbe40 Jean-François Lessard 2025-08-31 65 static struct linedisp *delete_attachment(struct device *dev, bool owns_device)
5b645f9dfbbe40 Jean-François Lessard 2025-08-31 66 {
5b645f9dfbbe40 Jean-François Lessard 2025-08-31 67 struct linedisp_attachment *attachment;
5b645f9dfbbe40 Jean-François Lessard 2025-08-31 68 struct linedisp *linedisp;
5b645f9dfbbe40 Jean-François Lessard 2025-08-31 69
5b645f9dfbbe40 Jean-François Lessard 2025-08-31 70 guard(spinlock)(&linedisp_attachments_lock);
5b645f9dfbbe40 Jean-François Lessard 2025-08-31 71
5b645f9dfbbe40 Jean-François Lessard 2025-08-31 72 list_for_each_entry(attachment, &linedisp_attachments, list) {
5b645f9dfbbe40 Jean-François Lessard 2025-08-31 73 if (attachment->device == dev &&
5b645f9dfbbe40 Jean-François Lessard 2025-08-31 74 attachment->owns_device == owns_device)
5b645f9dfbbe40 Jean-François Lessard 2025-08-31 75 break;
5b645f9dfbbe40 Jean-François Lessard 2025-08-31 76 }
5b645f9dfbbe40 Jean-François Lessard 2025-08-31 77
5b645f9dfbbe40 Jean-François Lessard 2025-08-31 78 if (list_entry_is_head(attachment, &linedisp_attachments, list))
5b645f9dfbbe40 Jean-François Lessard 2025-08-31 79 return NULL;
5b645f9dfbbe40 Jean-François Lessard 2025-08-31 80
5b645f9dfbbe40 Jean-François Lessard 2025-08-31 @81 linedisp = attachment->linedisp;
5b645f9dfbbe40 Jean-François Lessard 2025-08-31 82 list_del(&attachment->list);
5b645f9dfbbe40 Jean-François Lessard 2025-08-31 83 kfree(attachment);
5b645f9dfbbe40 Jean-François Lessard 2025-08-31 84
5b645f9dfbbe40 Jean-François Lessard 2025-08-31 85 return linedisp;
5b645f9dfbbe40 Jean-François Lessard 2025-08-31 86 }
5b645f9dfbbe40 Jean-François Lessard 2025-08-31 87
edd8dbd6f21b41 Jean-François Lessard 2025-08-31 88 static struct linedisp *to_linedisp(struct device *dev)
edd8dbd6f21b41 Jean-François Lessard 2025-08-31 89 {
5b645f9dfbbe40 Jean-François Lessard 2025-08-31 90 struct linedisp_attachment *attachment;
5b645f9dfbbe40 Jean-François Lessard 2025-08-31 91
5b645f9dfbbe40 Jean-François Lessard 2025-08-31 92 guard(spinlock)(&linedisp_attachments_lock);
5b645f9dfbbe40 Jean-François Lessard 2025-08-31 93
5b645f9dfbbe40 Jean-François Lessard 2025-08-31 94 list_for_each_entry(attachment, &linedisp_attachments, list) {
5b645f9dfbbe40 Jean-François Lessard 2025-08-31 95 if (attachment->device == dev)
5b645f9dfbbe40 Jean-François Lessard 2025-08-31 96 break;
5b645f9dfbbe40 Jean-François Lessard 2025-08-31 97 }
5b645f9dfbbe40 Jean-François Lessard 2025-08-31 98
5b645f9dfbbe40 Jean-François Lessard 2025-08-31 99 if (list_entry_is_head(attachment, &linedisp_attachments, list))
5b645f9dfbbe40 Jean-François Lessard 2025-08-31 100 return NULL;
5b645f9dfbbe40 Jean-François Lessard 2025-08-31 101
5b645f9dfbbe40 Jean-François Lessard 2025-08-31 @102 return attachment->linedisp;
edd8dbd6f21b41 Jean-François Lessard 2025-08-31 103 }
edd8dbd6f21b41 Jean-François Lessard 2025-08-31 104
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 0/5] auxdisplay: linedisp: support attribute attachment to auxdisplay devices
@ 2025-09-01 2:00 Jean-François Lessard
2025-09-01 2:00 ` [PATCH 4/5] " Jean-François Lessard
0 siblings, 1 reply; 6+ messages in thread
From: Jean-François Lessard @ 2025-09-01 2:00 UTC (permalink / raw)
To: Andy Shevchenko, Geert Uytterhoeven; +Cc: linux-kernel
This series modernizes the auxdisplay line display (linedisp) library to
enable seamless integration with auxdisplay parent devices while
maintaining backward compatibility.
The key improvement is adding attach/detach APIs that allow linedisp sysfs
attributes to be bound directly to their parent auxdisplay devices avoiding
child device proliferation and enabling a uniform 7-segment userspace
interface across different driver architectures.
This series introduces attachment infrastructure for linedisp devices.
The first consumer of this API will be the TM16XX driver series.
See the related patch series:
auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver
Changes include:
1. Encapsulate container_of() usage with to_linedisp() helper function for
cleaner context retrieval
2. Improve message display behavior with static padding when message length
is smaller than display width
3. Add 'num_chars' read-only attribute for userspace capability discovery
4. Add attach/detach API for sysfs attributes binding to parent devices
5. Document all linedisp sysfs attributes in ABI documentation
All existing linedisp_register() users remain unaffected. The new APIs
enable drivers like TM16XX to integrate 7-segment functionality within
their LED class device hierarchy while providing a uniform 7-segment API.
Thanks to Andy Shevchenko for early feedback and guidance.
RFC changelog:
- Replace scope_guard() with guard()() for synchronized list operations.
- Replace NULL assignments with proper list_entry_is_head() pattern.
- Clearly document why introducing the attach/detach APIs.
- Split in patch series, each patch containing a specific change.
- Implement static (non-scrolling) display for short messages.
- Document exisiting and new ABI sysfs attributes.
Jean-François Lessard (5):
auxdisplay: linedisp: encapsulate container_of usage within
to_linedisp
auxdisplay: linedisp: display static message when length <= display
size
auxdisplay: linedisp: add num_chars sysfs attribute
auxdisplay: linedisp: support attribute attachment to auxdisplay
devices
docs: ABI: auxdisplay: document linedisp library sysfs attributes
.../ABI/testing/sysfs-auxdisplay-linedisp | 90 +++++++
drivers/auxdisplay/line-display.c | 219 ++++++++++++++++--
drivers/auxdisplay/line-display.h | 4 +
3 files changed, 295 insertions(+), 18 deletions(-)
create mode 100644 Documentation/ABI/testing/sysfs-auxdisplay-linedisp
--
2.43.0
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 4/5] auxdisplay: linedisp: support attribute attachment to auxdisplay devices 2025-09-01 2:00 [PATCH 0/5] " Jean-François Lessard @ 2025-09-01 2:00 ` Jean-François Lessard 2025-09-02 10:18 ` Andy Shevchenko 0 siblings, 1 reply; 6+ messages in thread From: Jean-François Lessard @ 2025-09-01 2:00 UTC (permalink / raw) To: Andy Shevchenko, Geert Uytterhoeven; +Cc: linux-kernel Enable linedisp library integration into existing kernel devices (like LED class) to provide a uniform 7-segment userspace API without creating separate child devices, meeting the consistent interface while maintaining coherent device hierarchies. This allows uniform 7-segment API across all drivers while solving device proliferation and fragmented userspace interfaces. The provided attributes appear in two locations depending on usage: 1. On linedisp.N child devices (legacy linedisp_register()) 2. On the parent auxdisplay device (new linedisp_attach()) Functionality is identical in both modes. Existing consumers of linedisp_register() are unaffected. The new API enables drivers like TM16XX to integrate 7-segment display functionality seamlessly within their LED class device hierarchy. Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com> --- drivers/auxdisplay/line-display.c | 160 +++++++++++++++++++++++++++++- drivers/auxdisplay/line-display.h | 4 + 2 files changed, 161 insertions(+), 3 deletions(-) diff --git a/drivers/auxdisplay/line-display.c b/drivers/auxdisplay/line-display.c index abeed8812..1176d46f0 100644 --- a/drivers/auxdisplay/line-display.c +++ b/drivers/auxdisplay/line-display.c @@ -6,20 +6,23 @@ * Author: Paul Burton <paul.burton@mips.com> * * Copyright (C) 2021 Glider bv + * Copyright (C) 2025 Jean-François Lessard */ #ifndef CONFIG_PANEL_BOOT_MESSAGE #include <generated/utsrelease.h> #endif -#include <linux/container_of.h> +#include <linux/cleanup.h> #include <linux/device.h> #include <linux/export.h> #include <linux/idr.h> #include <linux/jiffies.h> #include <linux/kstrtox.h> +#include <linux/list.h> #include <linux/module.h> #include <linux/slab.h> +#include <linux/spinlock.h> #include <linux/string.h> #include <linux/sysfs.h> #include <linux/timer.h> @@ -31,9 +34,72 @@ #define DEFAULT_SCROLL_RATE (HZ / 2) +struct linedisp_attachment { + struct list_head list; + struct device *device; + struct linedisp *linedisp; + bool owns_device; /* true for child device mode, false for attached mode */ +}; + +static LIST_HEAD(linedisp_attachments); +static DEFINE_SPINLOCK(linedisp_attachments_lock); + +static int create_attachment(struct device *dev, struct linedisp *linedisp, bool owns_device) +{ + struct linedisp_attachment *attachment; + + attachment = kzalloc(sizeof(*attachment), GFP_KERNEL); + if (!attachment) + return -ENOMEM; + + attachment->device = dev; + attachment->linedisp = linedisp; + attachment->owns_device = owns_device; + + guard(spinlock)(&linedisp_attachments_lock); + list_add(&attachment->list, &linedisp_attachments); + + return 0; +} + +static struct linedisp *delete_attachment(struct device *dev, bool owns_device) +{ + struct linedisp_attachment *attachment; + struct linedisp *linedisp; + + guard(spinlock)(&linedisp_attachments_lock); + + list_for_each_entry(attachment, &linedisp_attachments, list) { + if (attachment->device == dev && + attachment->owns_device == owns_device) + break; + } + + if (list_entry_is_head(attachment, &linedisp_attachments, list)) + return NULL; + + linedisp = attachment->linedisp; + list_del(&attachment->list); + kfree(attachment); + + return linedisp; +} + static struct linedisp *to_linedisp(struct device *dev) { - return container_of(dev, struct linedisp, dev); + struct linedisp_attachment *attachment; + + guard(spinlock)(&linedisp_attachments_lock); + + list_for_each_entry(attachment, &linedisp_attachments, list) { + if (attachment->device == dev) + break; + } + + if (list_entry_is_head(attachment, &linedisp_attachments, list)) + return NULL; + + return attachment->linedisp; } static inline bool should_scroll(struct linedisp *linedisp) @@ -349,6 +415,87 @@ static int linedisp_init_map(struct linedisp *linedisp) #define LINEDISP_INIT_TEXT "Linux " UTS_RELEASE " " #endif +/** + * linedisp_attach - attach a character line display + * @linedisp: pointer to character line display structure + * @dev: pointer of the device to attach to + * @num_chars: the number of characters that can be displayed + * @ops: character line display operations + * + * Return: zero on success, else a negative error code. + */ +int linedisp_attach(struct linedisp *linedisp, struct device *dev, + unsigned int num_chars, const struct linedisp_ops *ops) +{ + int err; + + memset(linedisp, 0, sizeof(*linedisp)); + linedisp->ops = ops; + linedisp->num_chars = num_chars; + linedisp->scroll_rate = DEFAULT_SCROLL_RATE; + + linedisp->buf = kzalloc(linedisp->num_chars, GFP_KERNEL); + if (!linedisp->buf) + return -ENOMEM; + + /* initialise a character mapping, if required */ + err = linedisp_init_map(linedisp); + if (err) + goto out_free_buf; + + /* initialise a timer for scrolling the message */ + timer_setup(&linedisp->timer, linedisp_scroll, 0); + + err = create_attachment(dev, linedisp, false); + if (err) + goto out_del_timer; + + /* add attribute groups to target device */ + err = device_add_groups(dev, linedisp_groups); + if (err) + goto out_del_attach; + + /* display a default message */ + err = linedisp_display(linedisp, LINEDISP_INIT_TEXT, -1); + if (err) + goto out_rem_groups; + + return 0; + +out_rem_groups: + device_remove_groups(dev, linedisp_groups); +out_del_attach: + delete_attachment(dev, false); +out_del_timer: + timer_delete_sync(&linedisp->timer); +out_free_buf: + kfree(linedisp->buf); + return err; +} +EXPORT_SYMBOL_NS_GPL(linedisp_attach, "LINEDISP"); + +/** + * linedisp_detach - detach a character line display + * @dev: pointer of the device to detach from, that was previously + * attached with linedisp_attach() + */ +void linedisp_detach(struct device *dev) +{ + struct linedisp *linedisp = delete_attachment(dev, false); + + if (!linedisp) + return; + + timer_delete_sync(&linedisp->timer); + + device_remove_groups(dev, linedisp_groups); + + kfree(linedisp->map); + kfree(linedisp->message); + kfree(linedisp->buf); +} +EXPORT_SYMBOL_NS_GPL(linedisp_detach, "LINEDISP"); + /** * linedisp_register - register a character line display * @linedisp: pointer to character line display structure @@ -391,10 +538,14 @@ int linedisp_register(struct linedisp *linedisp, struct device *parent, /* initialise a timer for scrolling the message */ timer_setup(&linedisp->timer, linedisp_scroll, 0); - err = device_add(&linedisp->dev); + err = create_attachment(&linedisp->dev, linedisp, true); if (err) goto out_del_timer; + err = device_add(&linedisp->dev); + if (err) + goto out_del_attach; + /* display a default message */ err = linedisp_display(linedisp, LINEDISP_INIT_TEXT, -1); if (err) @@ -404,6 +555,8 @@ int linedisp_register(struct linedisp *linedisp, struct device *parent, out_del_dev: device_del(&linedisp->dev); +out_del_attach: + delete_attachment(&linedisp->dev, true); out_del_timer: timer_delete_sync(&linedisp->timer); out_put_device: @@ -420,6 +573,7 @@ EXPORT_SYMBOL_NS_GPL(linedisp_register, "LINEDISP"); void linedisp_unregister(struct linedisp *linedisp) { device_del(&linedisp->dev); + delete_attachment(&linedisp->dev, true); timer_delete_sync(&linedisp->timer); put_device(&linedisp->dev); } diff --git a/drivers/auxdisplay/line-display.h b/drivers/auxdisplay/line-display.h index 4348d7a2f..36853b639 100644 --- a/drivers/auxdisplay/line-display.h +++ b/drivers/auxdisplay/line-display.h @@ -6,6 +6,7 @@ * Author: Paul Burton <paul.burton@mips.com> * * Copyright (C) 2021 Glider bv + * Copyright (C) 2025 Jean-François Lessard */ #ifndef _LINEDISP_H @@ -81,6 +82,9 @@ struct linedisp { unsigned int id; }; +int linedisp_attach(struct linedisp *linedisp, struct device *dev, + unsigned int num_chars, const struct linedisp_ops *ops); +void linedisp_detach(struct device *dev); int linedisp_register(struct linedisp *linedisp, struct device *parent, unsigned int num_chars, const struct linedisp_ops *ops); void linedisp_unregister(struct linedisp *linedisp); -- 2.43.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 4/5] auxdisplay: linedisp: support attribute attachment to auxdisplay devices 2025-09-01 2:00 ` [PATCH 4/5] " Jean-François Lessard @ 2025-09-02 10:18 ` Andy Shevchenko 2025-09-02 17:37 ` Jean-François Lessard 0 siblings, 1 reply; 6+ messages in thread From: Andy Shevchenko @ 2025-09-02 10:18 UTC (permalink / raw) To: Jean-François Lessard Cc: Andy Shevchenko, Geert Uytterhoeven, linux-kernel On Sun, Aug 31, 2025 at 10:00:28PM -0400, Jean-François Lessard wrote: > Enable linedisp library integration into existing kernel devices (like LED > class) to provide a uniform 7-segment userspace API without creating > separate child devices, meeting the consistent interface while maintaining > coherent device hierarchies. > > This allows uniform 7-segment API across all drivers while solving device > proliferation and fragmented userspace interfaces. > > The provided attributes appear in two locations depending on usage: You wanted to say "...in one of the two possible..."? Otherwise it looks like undesired side effect of the change. > 1. On linedisp.N child devices (legacy linedisp_register()) > 2. On the parent auxdisplay device (new linedisp_attach()) > Functionality is identical in both modes. > > Existing consumers of linedisp_register() are unaffected. The new API > enables drivers like TM16XX to integrate 7-segment display functionality > seamlessly within their LED class device hierarchy. ... > +struct linedisp_attachment { > + struct list_head list; > + struct device *device; > + struct linedisp *linedisp; > + bool owns_device; /* true for child device mode, false for attached mode */ I would rename this to bool attached; // with inverted logic or bool mode; // with "default" (false) mode to be actually legacy one (so in both cases I think we want false for the legacy mode). > +}; ... > +static DEFINE_SPINLOCK(linedisp_attachments_lock); Why spin lock and not mutex? ... > +/** > + * linedisp_attach - attach a character line display > + * @linedisp: pointer to character line display structure > + * @dev: pointer of the device to attach to > + * @num_chars: the number of characters that can be displayed > + * @ops: character line display operations > + * Can you add a description here, please? Important note that it should be freed by the respective API afterwards. > + * Return: zero on success, else a negative error code. > + */ ... > + /* add attribute groups to target device */ > + err = device_add_groups(dev, linedisp_groups); > + if (err) > + goto out_del_attach; > + > + /* display a default message */ > + err = linedisp_display(linedisp, LINEDISP_INIT_TEXT, -1); > + if (err) > + goto out_rem_groups; Can this be racy with user space? Once we publish attributes, the new message can be already issued and here it puts another message. OTOH this is done before device_add(), so the attributes are not visible until the device is added. ... > +void linedisp_detach(struct device *dev) > +{ > + struct linedisp *linedisp = delete_attachment(dev, false); > + > + if (!linedisp) > + return; Please, rewrite as struct linedisp *linedisp; linedisp = delete_attachment(dev, false); if (!linedisp) return; > + timer_delete_sync(&linedisp->timer); > + > + device_remove_groups(dev, linedisp_groups); > + > + kfree(linedisp->map); > + kfree(linedisp->message); > + kfree(linedisp->buf); > +} -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 4/5] auxdisplay: linedisp: support attribute attachment to auxdisplay devices 2025-09-02 10:18 ` Andy Shevchenko @ 2025-09-02 17:37 ` Jean-François Lessard 2025-09-03 10:18 ` Andy Shevchenko 0 siblings, 1 reply; 6+ messages in thread From: Jean-François Lessard @ 2025-09-02 17:37 UTC (permalink / raw) To: Andy Shevchenko; +Cc: Andy Shevchenko, Geert Uytterhoeven, linux-kernel Le 2 septembre 2025 06 h 18 min 24 s HAE, Andy Shevchenko <andriy.shevchenko@intel.com> a écrit : >On Sun, Aug 31, 2025 at 10:00:28PM -0400, Jean-François Lessard wrote: >> Enable linedisp library integration into existing kernel devices (like LED >> class) to provide a uniform 7-segment userspace API without creating >> separate child devices, meeting the consistent interface while maintaining >> coherent device hierarchies. >> >> This allows uniform 7-segment API across all drivers while solving device >> proliferation and fragmented userspace interfaces. >> >> The provided attributes appear in two locations depending on usage: > >You wanted to say "...in one of the two possible..."? >Otherwise it looks like undesired side effect of the change. > Yes, your wording is much clearer. I will rephrase: The provided attributes appear in one of the two locations depending on usage: >> 1. On linedisp.N child devices (legacy linedisp_register()) >> 2. On the parent auxdisplay device (new linedisp_attach()) >> Functionality is identical in both modes. >> >> Existing consumers of linedisp_register() are unaffected. The new API >> enables drivers like TM16XX to integrate 7-segment display functionality >> seamlessly within their LED class device hierarchy. > >... > >> +struct linedisp_attachment { >> + struct list_head list; >> + struct device *device; >> + struct linedisp *linedisp; > >> + bool owns_device; /* true for child device mode, false for attached mode */ > >I would rename this to > > bool attached; // with inverted logic > >or > bool mode; // with "default" (false) mode to be actually legacy one > >(so in both cases I think we want false for the legacy mode). > Understood. I will rename, invert logic and also document as kernel-doc instead of inline comment. >> +}; > >... > >> +static DEFINE_SPINLOCK(linedisp_attachments_lock); > >Why spin lock and not mutex? > The attachment list operations are extremely lightweight (just adding/removing list entries), making spinlock the optimal choice because: - Very short critical sections: Only list traversal and pointer assignments; avoids context switching overhead for brief operations - No sleeping operations: No memory allocation or I/O within locked sections - Future-proof atomic context safety: Can be safely called from interrupt handlers if needed >... > >> +/** >> + * linedisp_attach - attach a character line display >> + * @linedisp: pointer to character line display structure >> + * @dev: pointer of the device to attach to >> + * @num_chars: the number of characters that can be displayed >> + * @ops: character line display operations >> + * > >Can you add a description here, please? Important note that it should be freed >by the respective API afterwards. > Yes, I will clarify that attach/register operations must be freed using their corresponding detach/unregister operations. >> + * Return: zero on success, else a negative error code. >> + */ > >... > >> + /* add attribute groups to target device */ >> + err = device_add_groups(dev, linedisp_groups); >> + if (err) >> + goto out_del_attach; >> + >> + /* display a default message */ >> + err = linedisp_display(linedisp, LINEDISP_INIT_TEXT, -1); >> + if (err) >> + goto out_rem_groups; > >Can this be racy with user space? Once we publish attributes, the new >message can be already issued and here it puts another message. >OTOH this is done before device_add(), so the attributes are not visible >until the device is added. > This concern is perfectly valid. linedisp_attach() can and will be called after device_add() which can be racy. In fact, current linedisp_attach() replicates the linedisp_register() logic which is also racy since it displays initial message after device_add(). It needs to be fixed in both attach/register cases by first initializing the display message before calling device_add_groups()/device_add(). >... > >> +void linedisp_detach(struct device *dev) >> +{ > >> + struct linedisp *linedisp = delete_attachment(dev, false); >> + >> + if (!linedisp) >> + return; > >Please, rewrite as > > struct linedisp *linedisp; > > linedisp = delete_attachment(dev, false); > if (!linedisp) > return; > Acknowledged. I will separate assignment from declaration. >> + timer_delete_sync(&linedisp->timer); >> + >> + device_remove_groups(dev, linedisp_groups); >> + >> + kfree(linedisp->map); >> + kfree(linedisp->message); >> + kfree(linedisp->buf); >> +} > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 4/5] auxdisplay: linedisp: support attribute attachment to auxdisplay devices 2025-09-02 17:37 ` Jean-François Lessard @ 2025-09-03 10:18 ` Andy Shevchenko 2025-09-03 11:31 ` Jean-François Lessard 0 siblings, 1 reply; 6+ messages in thread From: Andy Shevchenko @ 2025-09-03 10:18 UTC (permalink / raw) To: Jean-François Lessard Cc: Andy Shevchenko, Geert Uytterhoeven, linux-kernel On Tue, Sep 02, 2025 at 01:37:52PM -0400, Jean-François Lessard wrote: > Le 2 septembre 2025 06 h 18 min 24 s HAE, Andy Shevchenko <andriy.shevchenko@intel.com> a écrit : > >On Sun, Aug 31, 2025 at 10:00:28PM -0400, Jean-François Lessard wrote: ... > >> +static DEFINE_SPINLOCK(linedisp_attachments_lock); > > > >Why spin lock and not mutex? > > The attachment list operations are extremely lightweight (just adding/removing > list entries), making spinlock the optimal choice because: > - Very short critical sections: Only list traversal and pointer assignments; > avoids context switching overhead for brief operations > - No sleeping operations: No memory allocation or I/O within locked sections > - Future-proof atomic context safety: Can be safely called from interrupt > handlers if needed To me it sounds like solving non-existing problem. I am sure we will see no driver that tries to call this API in an atomic context. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 4/5] auxdisplay: linedisp: support attribute attachment to auxdisplay devices 2025-09-03 10:18 ` Andy Shevchenko @ 2025-09-03 11:31 ` Jean-François Lessard 0 siblings, 0 replies; 6+ messages in thread From: Jean-François Lessard @ 2025-09-03 11:31 UTC (permalink / raw) To: Andy Shevchenko; +Cc: Andy Shevchenko, Geert Uytterhoeven, linux-kernel Le 3 septembre 2025 06 h 18 min 18 s HAE, Andy Shevchenko <andriy.shevchenko@intel.com> a écrit : >On Tue, Sep 02, 2025 at 01:37:52PM -0400, Jean-François Lessard wrote: >> Le 2 septembre 2025 06 h 18 min 24 s HAE, Andy Shevchenko <andriy.shevchenko@intel.com> a écrit : >> >On Sun, Aug 31, 2025 at 10:00:28PM -0400, Jean-François Lessard wrote: > >... > >> >> +static DEFINE_SPINLOCK(linedisp_attachments_lock); >> > >> >Why spin lock and not mutex? >> >> The attachment list operations are extremely lightweight (just adding/removing >> list entries), making spinlock the optimal choice because: >> - Very short critical sections: Only list traversal and pointer assignments; >> avoids context switching overhead for brief operations >> - No sleeping operations: No memory allocation or I/O within locked sections >> - Future-proof atomic context safety: Can be safely called from interrupt >> handlers if needed > >To me it sounds like solving non-existing problem. I am sure we will see no >driver that tries to call this API in an atomic context. > Yeah, I should have skipped "Future-proof atomic context safety". I don't see how attach/detach would be called from atomic context. Maybe to_linedisp(), but not in the current implementation anyway. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-09-03 11:31 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-09-02 10:38 [PATCH 4/5] auxdisplay: linedisp: support attribute attachment to auxdisplay devices kernel test robot -- strict thread matches above, loose matches on Subject: below -- 2025-09-01 2:00 [PATCH 0/5] " Jean-François Lessard 2025-09-01 2:00 ` [PATCH 4/5] " Jean-François Lessard 2025-09-02 10:18 ` Andy Shevchenko 2025-09-02 17:37 ` Jean-François Lessard 2025-09-03 10:18 ` Andy Shevchenko 2025-09-03 11:31 ` Jean-François Lessard
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.