Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 17 Sep 2021 06:53:26 +0100
From:   Christoph Hellwig <hch@infradead.org>
To:     Ian Pilcher <arequipeno@gmail.com>
Cc:     pavel@ucw.cz, linux-leds@vger.kernel.org,
        linux-kernel@vger.kernel.org, gregkh@linuxfoundation.org,
        kabel@kernel.org
Subject: Re: [PATCH v4 2/2] leds: trigger: Add block device LED trigger
Message-ID: <YUQtVmwV90uab4/V@infradead.org>
References: <20210916202127.1216994-1-arequipeno@gmail.com>
 <20210916202127.1216994-3-arequipeno@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210916202127.1216994-3-arequipeno@gmail.com>
Precedence: bulk

On Thu, Sep 16, 2021 at 03:21:27PM -0500, Ian Pilcher wrote:
> +/* Block device information (BDI) - 1 per blkdev linked to at least 1 LED */
> +struct led_bdev_bdi {

It might be a good idea to pick a differene name.  BDI is a commonly
used shortcut for the backing device information used all over the block
layer and writeback code.

> +/* For many-to-many relationships between block devices and LEDs */
> +struct led_bdev_link {
> +	struct hlist_node	bdi_leds_node;
> +	struct hlist_node	led_bdis_node;
> +	struct led_bdev_bdi	*bdi;
> +	struct led_bdev_led	*led;
> +};


Why not just use a xarray to link them which due to the non-invasive
nature gets you n:m links "for free".

> +/* Forward declarations to make this file compile in a reasonable order */
> +static void led_bdev_process(struct work_struct *work);
> +static struct led_bdev_bdi *led_bdev_get_bdi(const char *buf, size_t size);
> +static struct block_device *led_bdev_get(const char *buf, size_t size,
> +					 fmode_t mode);
> +static int led_bdev_link(struct led_bdev_led *led, struct led_bdev_bdi *bdi);
> +static void led_bdev_put_bdi(struct led_bdev_bdi *bdi);
> +static void led_bdev_bdi_release(struct device *dev, void *res);
> +static void led_bdev_unlink(struct led_bdev_led *led,
> +			    struct led_bdev_link *link,
> +			    struct led_bdev_bdi *bdi, bool releasing);
> +static void led_bdev_update_bdi(struct led_bdev_bdi *bdi);
> +static bool led_bdev_blink(const struct led_bdev_led *led,
> +			   const struct led_bdev_bdi *bdi);

I seriously question the "reasonable" above if you need that many
forward declarations in brand new code.

> +static struct block_device *led_bdev_get(const char *const buf,
> +					 const size_t count, fmode_t mode)
> +{
> +	static const char dev[] = "/dev/";
> +	struct block_device *bdev;
> +	char *dev_path, *path;
> +
> +	/* sizeof(dev) includes terminating null */
> +	dev_path = kmalloc(sizeof(dev) + count, GFP_KERNEL);
> +	if (dev_path == NULL)
> +		return ERR_PTR(-ENOMEM);
> +
> +	/* sizeof(dev) - 1 is compile-time equivalent of strlen(dev) */
> +	memcpy(dev_path, dev, sizeof(dev) - 1);
> +	path = dev_path + sizeof(dev) - 1;
> +	memcpy(path, buf, count + 1);  /* include terminating null */
> +	strim(path);
> +
> +try_blkdev_get:
> +	bdev = blkdev_get_by_path(path, mode, THIS_MODULE);
> +	if (IS_ERR(bdev) && PTR_ERR(bdev) == -ENOENT && path != dev_path) {
> +		path = dev_path;
> +		goto try_blkdev_get;
> +	}
> +
> +	kfree(dev_path);
> +	return bdev;

Please just required the user to put in the whole path and remove all
this garbage.  There is no need to build your own broken wrappers around
the VFS path resolution.