Re: [PATCH 15/18] ledtrig-blkdev: Add sysfs attributes to [un]link LEDs & devices

[Greg KH <gregkh@linuxfoundation.org>]

On Sat, Sep 04, 2021 at 05:35:29PM -0500, Ian Pilcher wrote:
> On 9/4/21 12:59 AM, Greg KH wrote:
> > DEVICE_ATTR_RO()?  Or something like that?  Do not use __ATTR() for
> > device attributes if at all possible, worst case, use DEVICE_ATTR()
> > here.
> 
> For some reason, it didn't click until now that these are device
> attributes (because I was focused on the fact that I was working on the
> LED trigger).
> 
> DEVICE_ATTR*() it is.
> 
> > And the mode settings are odd, are you sure you want that?
> 
> Yes.  These are write-only attributes.

DEVICE_ATTR_WO() then please.

> > > +	if (name_len == 0) {
> > > +		pr_info("blkdev LED: empty block device name\n");
> > 
> > Looks like debugging code, please remove.
> 
> It's really more of an error message for the system administrator.  So
> as with my earlier note, dev_info() would be my preference.

Nope, dev_err() for real errors please.

> > And how can this ever happen?
> 
> The blkdev_skip_space() and blkdev_find_space() calls effectively find
> the first non-whitespace token in the buffer (disk_name) and its length
> (name_len).  If the buffer only contains whitespace (e.g. echo > $ATTR),
> then name_len will be 0.

That's a crazy interface, as others pointed out, don't do that please.

thanks,

greg k-h