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
