| Message ID | 20210903204548.2745354-1-arequipeno@gmail.com |
|---|---|
| Headers | show |
| Series | Introduce block device LED trigger | expand |
On Fri, Sep 03, 2021 at 03:45:47PM -0500, Ian Pilcher wrote: > +static ssize_t blkdev_mode_store(struct device *const dev, > + struct device_attribute *const attr, > + const char *const buf, const size_t count) > +{ > + struct ledtrig_blkdev_led *const led = led_trigger_get_drvdata(dev); > + const char *const mode_name = blkdev_skip_space(buf); > + const char *const endp = blkdev_find_space(mode_name); > + const ptrdiff_t name_len = endp - mode_name; /* always >= 0 */ > + enum ledtrig_blkdev_mode mode; > + > + if (name_len == 0) { > + pr_info("blkdev LED: empty mode\n"); > + return -EINVAL; > + } > + > + for (mode = LEDTRIG_BLKDEV_MODE_RO; > + mode <= LEDTRIG_BLKDEV_MODE_RW; ++mode) { > + > + if (ledtrig_blkdev_streq(blkdev_modes[mode].name, > + mode_name, name_len)) { > + WRITE_ONCE(led->mode, mode); > + return count; > + } > + } > + > + pr_info("blkdev LED: invalid mode (%.*s)\n", (int)name_len, mode_name); Please do not use pr_* calls in a driver when you have a struct device. Also, you are now allowing any user to spam the kernel log, this shold be a dev_dbg() call at the most, if it is even needed at all. Same for the other pr_info() call in this function, please remove them all. thanks, greg k-h
On Fri, Sep 03, 2021 at 03:45:39PM -0500, Ian Pilcher wrote: > Use a delayed workqueue to periodically check configured block devices for > activity since the last check. Blink LEDs associated with devices on which > the configured type of activity (read/write) has occurred. > > Signed-off-by: Ian Pilcher <arequipeno@gmail.com> > --- > drivers/leds/trigger/ledtrig-blkdev.c | 88 +++++++++++++++++++++++++++ > 1 file changed, 88 insertions(+) > > diff --git a/drivers/leds/trigger/ledtrig-blkdev.c b/drivers/leds/trigger/ledtrig-blkdev.c > index 1f319529c3be..37ba9bb3542e 100644 > --- a/drivers/leds/trigger/ledtrig-blkdev.c > +++ b/drivers/leds/trigger/ledtrig-blkdev.c > @@ -7,7 +7,9 @@ > */ > > #include <linux/ctype.h> > +#include <linux/leds.h> > #include <linux/module.h> > +#include <linux/part_stat.h> > > #include "ledtrig-blkdev.h" > > @@ -68,6 +70,10 @@ static unsigned int ledtrig_blkdev_count; > /* How often to check for drive activity - in jiffies */ > static unsigned int ledtrig_blkdev_interval; > > +/* Delayed work used to periodically check for activity & blink LEDs */ > +static void blkdev_process(struct work_struct *const work); > +static DECLARE_DELAYED_WORK(ledtrig_blkdev_work, blkdev_process); > + > > /* > * > @@ -110,3 +116,85 @@ static bool blkdev_write_mode(const enum ledtrig_blkdev_mode mode) > { > return mode != LEDTRIG_BLKDEV_MODE_RO; > } > + > + > +/* > + * > + * Periodically check for device acitivity and blink LEDs > + * > + */ > + > +static void blkdev_blink(const struct ledtrig_blkdev_led *const led) > +{ > + unsigned long delay_on = READ_ONCE(led->blink_msec); > + unsigned long delay_off = 1; /* 0 leaves LED turned on */ > + > + led_blink_set_oneshot(led->led_dev, &delay_on, &delay_off, 0); > +} > + > +static void blkdev_update_disk(struct ledtrig_blkdev_disk *const disk, > + const unsigned int generation) > +{ > + const struct block_device *const part0 = disk->gd->part0; > + const unsigned long read_ios = part_stat_read(part0, ios[STAT_READ]); > + const unsigned long write_ios = part_stat_read(part0, ios[STAT_WRITE]) > + + part_stat_read(part0, ios[STAT_DISCARD]) > + + part_stat_read(part0, ios[STAT_FLUSH]); > + > + if (disk->read_ios != read_ios) { > + disk->read_act = true; > + disk->read_ios = read_ios; > + } else { > + disk->read_act = false; > + } > + > + if (disk->write_ios != write_ios) { > + disk->write_act = true; > + disk->write_ios = write_ios; > + } else { > + disk->write_act = false; > + } > + > + disk->generation = generation; > +} > + > +static void blkdev_process(struct work_struct *const work) > +{ > + static unsigned int generation; > + > + struct ledtrig_blkdev_led *led; > + struct ledtrig_blkdev_link *link; > + unsigned long delay; > + > + if (!mutex_trylock(&ledtrig_blkdev_mutex)) > + goto exit_reschedule; > + > + hlist_for_each_entry(led, &ledtrig_blkdev_leds, leds_node) { > + > + hlist_for_each_entry(link, &led->disks, led_disks_node) { > + > + struct ledtrig_blkdev_disk *const disk = link->disk; > + > + if (disk->generation != generation) > + blkdev_update_disk(disk, generation); > + > + if (disk->read_act && blkdev_read_mode(led->mode)) { > + blkdev_blink(led); > + break; > + } > + > + if (disk->write_act && blkdev_write_mode(led->mode)) { > + blkdev_blink(led); > + break; > + } > + } > + } > + > + ++generation; > + > + mutex_unlock(&ledtrig_blkdev_mutex); > + > +exit_reschedule: > + delay = READ_ONCE(ledtrig_blkdev_interval); > + WARN_ON_ONCE(!schedule_delayed_work(&ledtrig_blkdev_work, delay)); You just rebooted a machine if it hit this :( Please never use WARN_ON() in new code unless the machine is really broken and you can not do anything else here. thanks, greg k-h
Hi! > This patch series adds a new "blkdev" LED trigger for disk (or other block > device) activity LEDs. > > It has the following functionality. Correct address for lkml is linux-kernel@vger, not linux@vger. Best regards, Pavel -- http://www.livejournal.com/~pavelmachek
On 9/4/21 12:57 AM, Greg KH wrote: > Also, you are now allowing any user to spam the kernel log, this shold > be a dev_dbg() call at the most, if it is even needed at all. Same for > the other pr_info() call in this function, please remove them all. Greg - A bit of a "philosophical" question, if I may ... Is allowing the root user to "spam" the kernel log really a concern? (The permissions on the attribute don't allow non-root users to write to it.) As a system administrator working with a sysfs API (or writing udev rules to do so), I know that I appreciate having a meaningful message in the log when something doesn't work. Given that only the root user can trigger these messages, would you be OK with dev_info()? -- ======================================================================== In Soviet Russia, Google searches you! ========================================================================
On 9/4/21 1:01 AM, Greg KH wrote: > Please never use WARN_ON() in new code unless the machine is really > broken and you can not do anything else here. Wait what? I thought that was BUG_ON. -- ======================================================================== In Soviet Russia, Google searches you! ========================================================================
On Sat, Sep 04, 2021 at 04:01:56PM -0500, Ian Pilcher wrote: > On 9/4/21 12:57 AM, Greg KH wrote: > > Also, you are now allowing any user to spam the kernel log, this shold > > be a dev_dbg() call at the most, if it is even needed at all. Same for > > the other pr_info() call in this function, please remove them all. > > Greg - > > A bit of a "philosophical" question, if I may ... > > Is allowing the root user to "spam" the kernel log really a concern? Yes. > (The permissions on the attribute don't allow non-root users to write > to it.) Ah, but that was not obvious :) > As a system administrator working with a sysfs API (or writing udev > rules to do so), I know that I appreciate having a meaningful message in > the log when something doesn't work. That's fine, but do not allow a "normal" user to do so please. > Given that only the root user can trigger these messages, would you be > OK with dev_info()? For a sysfs file failure, use dev_err(). If things are working properly, your kernel code should be totally silent. thanks, greg k-h
On Sun, Sep 05, 2021 at 09:39:57AM -0500, Ian Pilcher wrote: > On 9/4/21 1:01 AM, Greg KH wrote: > > Please never use WARN_ON() in new code unless the machine is really > > broken and you can not do anything else here. > > Wait what? I thought that was BUG_ON. Not whan panic-on-warn is set, which is getting more and more common these days. thanks, greg k-h
On 9/5/21 9:51 AM, Greg KH wrote: > On Sun, Sep 05, 2021 at 09:39:57AM -0500, Ian Pilcher wrote: >> On 9/4/21 1:01 AM, Greg KH wrote: >>> Please never use WARN_ON() in new code unless the machine is really >>> broken and you can not do anything else here. >> >> Wait what? I thought that was BUG_ON. > > Not whan panic-on-warn is set, which is getting more and more common > these days. Fair enough. What is the recommend approach to reporting a "this should never" happen situation these days? Thanks! -- ======================================================================== In Soviet Russia, Google searches you! ========================================================================
On Sun, Sep 05, 2021 at 09:56:58AM -0500, Ian Pilcher wrote: > On 9/5/21 9:51 AM, Greg KH wrote: > > On Sun, Sep 05, 2021 at 09:39:57AM -0500, Ian Pilcher wrote: > > > On 9/4/21 1:01 AM, Greg KH wrote: > > > > Please never use WARN_ON() in new code unless the machine is really > > > > broken and you can not do anything else here. > > > > > > Wait what? I thought that was BUG_ON. > > > > Not whan panic-on-warn is set, which is getting more and more common > > these days. > > Fair enough. What is the recommend approach to reporting a "this should > never" happen situation these days? dev_err() and handle the error properly.
On Sun, Sep 05, 2021 at 05:12:39PM +0200, Greg KH wrote: > On Sun, Sep 05, 2021 at 09:56:58AM -0500, Ian Pilcher wrote: > > On 9/5/21 9:51 AM, Greg KH wrote: > > > On Sun, Sep 05, 2021 at 09:39:57AM -0500, Ian Pilcher wrote: > > > > On 9/4/21 1:01 AM, Greg KH wrote: > > > > > Please never use WARN_ON() in new code unless the machine is really > > > > > broken and you can not do anything else here. > > > > > > > > Wait what? I thought that was BUG_ON. > > > > > > Not whan panic-on-warn is set, which is getting more and more common > > > these days. > > > > Fair enough. What is the recommend approach to reporting a "this should > > never" happen situation these days? > > dev_err() and handle the error properly. > > WARN_ON is the right choice for reporting recoverable kernel bugs, and BUG_ON for unrecoverable ones; see the two comments in include/asm-generic/bug.h which explain this. Please don't use dev_err() if it's a kernel bug (and not just unexpected input from userspace or hardware behaving weirdly), as that prevents the bug from being reported if it occurs. Greg, you've been corrected on this before, e.g. https://lore.kernel.org/linux-fsdevel/20210707023548.15872-1-desmondcheongzx@gmail.com/T/#u. Please stop spreading false information as it is destroying your credibility :-( - Eric
This patch series adds a new "blkdev" LED trigger for disk (or other block device) activity LEDs. It has the following functionality. * Supports all types of block devices, including virtual devices (unlike the existing disk trigger which only works with ATA devices). * LEDs can be configured to show read activity, write activity, or both. * Supports multiple devices and multiple LEDs in arbitrary many-to-many configurations. For example, it is possible to configure multiple devices with device-specific read activity LEDs and a shared write activity LED. (See Documentation/leds/ledtrig-blkdev.rst in the first patch.) * Doesn't add any overhead in the I/O path. Like the netdev LED trigger, it periodically checks the configured devices for activity and blinks its LEDs as appropriate. * Blink duration (per LED) and interval between activity checks (global) are configurable. * Requires minimal changes to the block subsystem. - Adds 1 pointer to struct gendisk, - Adds (inline function) call in device_add_disk() to ensure that the pointer is initialized to NULL (as protection against any drivers that allocate a gendisk themselves and don't use kzalloc()), and - Adds call in del_gendisk() to remove a device from the trigger when that device is being removed. These changes are all in patch #4, "block: Add block device LED trigger integrations." * The trigger can be mostly built as a module. When the trigger is modular, a small portion is built in to provide a "stub" function which can be called from del_gendisk(). The stub calls into the modular code via a function pointer when needed. The trigger also needs the ability to find gendisk's by name, which requires access to the un-exported block_class and disk_type symbols. NOTES: * This patch series applies cleanly to the linux-block and linux-next (20210903) trees. All patches other than the block subsystem patch (patch #4) apply cleanly to the linux-leds tree. * All patches compile (modulo warnings) with the trigger disabled, modular, or built-in. Ian Pilcher (18): docs: Add block device (blkdev) LED trigger documentation ledtrig-blkdev: Add build infra for block device LED trigger ledtrig-blkdev: Add function placeholders needed by block changes block: Add block device LED trigger integrations ledtrig-blkdev: Implement functions called from block subsystem ledtrig-blkdev: Add function to get gendisk by name ledtrig-blkdev: Add constants, data types, and global variables ledtrig-blkdev: Add miscellaneous helper functions ledtrig-blkdev: Periodically check devices for activity & blink LEDs ledtrig-blkdev: Add function to associate the trigger with an LED ledtrig-blkdev: Add function to associate a device with an LED ledtrig-blkdev: Add function to remove LED/device association ledtrig-blkdev: Add function to disassociate a device from all LEDs ledtrig-blkdev: Add function to disassociate an LED from the trigger ledtrig-blkdev: Add sysfs attributes to [un]link LEDs & devices ledtrig-blkdev: Add blink_time & interval sysfs attributes ledtrig-blkdev: Add mode (read/write/rw) sysfs attributue ledtrig-blkdev: Add initialization & exit functions Documentation/ABI/testing/sysfs-block | 9 + .../testing/sysfs-class-led-trigger-blkdev | 48 ++ Documentation/leds/index.rst | 1 + Documentation/leds/ledtrig-blkdev.rst | 144 ++++ block/genhd.c | 4 + drivers/leds/trigger/Kconfig | 18 + drivers/leds/trigger/Makefile | 2 + drivers/leds/trigger/ledtrig-blkdev-core.c | 78 ++ drivers/leds/trigger/ledtrig-blkdev.c | 767 ++++++++++++++++++ drivers/leds/trigger/ledtrig-blkdev.h | 27 + include/linux/genhd.h | 3 + include/linux/leds.h | 20 + 12 files changed, 1121 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-class-led-trigger-blkdev create mode 100644 Documentation/leds/ledtrig-blkdev.rst create mode 100644 drivers/leds/trigger/ledtrig-blkdev-core.c create mode 100644 drivers/leds/trigger/ledtrig-blkdev.c create mode 100644 drivers/leds/trigger/ledtrig-blkdev.h