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.
+ 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.
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.
+ return -EINVAL;
+ }
+
+ if (attr == &ledtrig_blkdev_attr_del) {
+ blkdev_disk_delete(led, disk_name, name_len);
+ } else { /* attr == &ledtrig_blkdev_attr_add */
+ ret = blkdev_disk_add(led, disk_name, name_len);
Why do you have a single attribute callback that does two totally
different things? Just have 2 different callback functions please, it
makes things much easier to review and maintain over time.
Hmmm. All of the "real work" is done in blkdev_disk_delete() and
blkdev_disk_add(). The store function's only purpose is to parse the
token(s) from the buffer, and that logic is exactly the same for the
two different attributes.
So it's a choice between:
static ssize_t blkdev_add_or_del(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 disk_name = blkdev_skip_space(buf);
const char *const endp = blkdev_find_space(disk_name);
const ptrdiff_t name_len = endp - disk_name; /* always >= 0 */
int ret;
if (name_len == 0) {
pr_info("blkdev LED: empty block device name\n");
return -EINVAL;
}
if (attr == &ledtrig_blkdev_attr_del) {
blkdev_disk_delete(led, disk_name, name_len);
} else { /* attr == &ledtrig_blkdev_attr_add */
ret = blkdev_disk_add(led, disk_name, name_len);
if (ret != 0)
return ret;
}
/*
* Consume everything up to the next non-whitespace token (or the end
* of the input). Avoids "empty block device name" error if there is
* whitespace (such as a newline) after the last token.
*/
return blkdev_skip_space(endp) - buf;
}
Or:
static ssize_t blkdev_add(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 disk_name = blkdev_skip_space(buf);
const char *const endp = blkdev_find_space(disk_name);
const ptrdiff_t name_len = endp - disk_name; /* always >= 0 */
int ret;
if (name_len == 0) {
pr_info("blkdev LED: empty block device name\n");
return -EINVAL;
}
ret = blkdev_disk_add(led, disk_name, name_len);
if (ret != 0)
return ret;
/*
* Consume everything up to the next non-whitespace token (or the end
* of the input). Avoids "empty block device name" error if there is
* whitespace (such as a newline) after the last token.
*/
return blkdev_skip_space(endp) - buf;
}
static ssize_t blkdev_del(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 disk_name = blkdev_skip_space(buf);
const char *const endp = blkdev_find_space(disk_name);
const ptrdiff_t name_len = endp - disk_name; /* always >= 0 */
int ret;
if (name_len == 0) {
pr_info("blkdev LED: empty block device name\n");
return -EINVAL;
}
blkdev_disk_delete(led, disk_name, name_len);
/* See comment in blkdev_add() */
return blkdev_skip_space(endp) - buf;
}
Maybe the right thing to do is to simply add a comment to clarify the
separation (and maybe rename the function as well)?
Thanks!
--
========================================================================
In Soviet Russia, Google searches you!
========================================================================
