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.

quoted

+	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.

quoted

+		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!
========================================================================