Re: [PATCH] dmaengine: fix channel index enumeration

[Dave Jiang <dave.jiang@xxxxxxxxx>]

On 4/13/2020 10:40 AM, Jiang, Dave wrote:
> When the channel register code was changed to allow hotplug operations,
> dynamic indexing wasn't taken into account. When channels are randomly
> plugged and unplugged out of order, the serial indexing breaks. Convert
> channel indexing to using IDA tracking in order to allow dynamic
> assignment. The previous code does not cause any regression bug for
> existing channel allocation besides idxd driver since the hotplug usage
> case is only used by idxd at this point.
>
> With this change, the chan->idr_ref is also not needed any longer. We can
> have a device with no channels registered due to hot plug. The channel
> device release code no longer should attempt to free the dma device id on
> the last channel release.

Hi Vinod, just checking to see if you had time to look at this patch 
yet. Thanks.

> Fixes: e81274cd6b52 ("dmaengine: add support to dynamic register/unregister of channels")
>
> Reported-by: Yixin Zhang <yixin.zhang@xxxxxxxxx>
> Signed-off-by: Dave Jiang <dave.jiang@xxxxxxxxx>
> Tested-by: Yixin Zhang <yixin.zhang@xxxxxxxxx>
> ---
>   drivers/dma/dmaengine.c   |   60 ++++++++++++++++++++-------------------------
>   include/linux/dmaengine.h |    4 ++-
>   2 files changed, 28 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
> index 72c584f1dd26..e341f8d7ae62 100644
> --- a/drivers/dma/dmaengine.c
> +++ b/drivers/dma/dmaengine.c
> @@ -151,10 +151,6 @@ static void chan_dev_release(struct device *dev)
>   	struct dma_chan_dev *chan_dev;
>   
>   	chan_dev = container_of(dev, typeof(*chan_dev), device);
> -	if (atomic_dec_and_test(chan_dev->idr_ref)) {
> -		ida_free(&dma_ida, chan_dev->dev_id);
> -		kfree(chan_dev->idr_ref);
> -	}
>   	kfree(chan_dev);
>   }
>   
> @@ -952,27 +948,9 @@ static int get_dma_id(struct dma_device *device)
>   }
>   
>   static int __dma_async_device_channel_register(struct dma_device *device,
> -					       struct dma_chan *chan,
> -					       int chan_id)
> +					       struct dma_chan *chan)
>   {
>   	int rc = 0;
> -	int chancnt = device->chancnt;
> -	atomic_t *idr_ref;
> -	struct dma_chan *tchan;
> -
> -	tchan = list_first_entry_or_null(&device->channels,
> -					 struct dma_chan, device_node);
> -	if (!tchan)
> -		return -ENODEV;
> -
> -	if (tchan->dev) {
> -		idr_ref = tchan->dev->idr_ref;
> -	} else {
> -		idr_ref = kmalloc(sizeof(*idr_ref), GFP_KERNEL);
> -		if (!idr_ref)
> -			return -ENOMEM;
> -		atomic_set(idr_ref, 0);
> -	}
>   
>   	chan->local = alloc_percpu(typeof(*chan->local));
>   	if (!chan->local)
> @@ -988,29 +966,36 @@ static int __dma_async_device_channel_register(struct dma_device *device,
>   	 * When the chan_id is a negative value, we are dynamically adding
>   	 * the channel. Otherwise we are static enumerating.
>   	 */
> -	chan->chan_id = chan_id < 0 ? chancnt : chan_id;
> +	mutex_lock(&device->chan_mutex);
> +	chan->chan_id = ida_alloc(&device->chan_ida, GFP_KERNEL);
> +	mutex_unlock(&device->chan_mutex);
> +	if (chan->chan_id < 0) {
> +		pr_err("%s: unable to alloc ida for chan: %d\n",
> +		       __func__, chan->chan_id);
> +		goto err_out;
> +	}
> +
>   	chan->dev->device.class = &dma_devclass;
>   	chan->dev->device.parent = device->dev;
>   	chan->dev->chan = chan;
> -	chan->dev->idr_ref = idr_ref;
>   	chan->dev->dev_id = device->dev_id;
> -	atomic_inc(idr_ref);
>   	dev_set_name(&chan->dev->device, "dma%dchan%d",
>   		     device->dev_id, chan->chan_id);
> -
>   	rc = device_register(&chan->dev->device);
>   	if (rc)
> -		goto err_out;
> +		goto err_out_ida;
>   	chan->client_count = 0;
> -	device->chancnt = chan->chan_id + 1;
> +	device->chancnt++;
>   
>   	return 0;
>   
> + err_out_ida:
> +	mutex_lock(&device->chan_mutex);
> +	ida_free(&device->chan_ida, chan->chan_id);
> +	mutex_unlock(&device->chan_mutex);
>    err_out:
>   	free_percpu(chan->local);
>   	kfree(chan->dev);
> -	if (atomic_dec_return(idr_ref) == 0)
> -		kfree(idr_ref);
>   	return rc;
>   }
>   
> @@ -1019,7 +1004,7 @@ int dma_async_device_channel_register(struct dma_device *device,
>   {
>   	int rc;
>   
> -	rc = __dma_async_device_channel_register(device, chan, -1);
> +	rc = __dma_async_device_channel_register(device, chan);
>   	if (rc < 0)
>   		return rc;
>   
> @@ -1039,6 +1024,9 @@ static void __dma_async_device_channel_unregister(struct dma_device *device,
>   	device->chancnt--;
>   	chan->dev->chan = NULL;
>   	mutex_unlock(&dma_list_mutex);
> +	mutex_lock(&device->chan_mutex);
> +	ida_free(&device->chan_ida, chan->chan_id);
> +	mutex_unlock(&device->chan_mutex);
>   	device_unregister(&chan->dev->device);
>   	free_percpu(chan->local);
>   }
> @@ -1061,7 +1049,7 @@ EXPORT_SYMBOL_GPL(dma_async_device_channel_unregister);
>    */
>   int dma_async_device_register(struct dma_device *device)
>   {
> -	int rc, i = 0;
> +	int rc;
>   	struct dma_chan* chan;
>   
>   	if (!device)
> @@ -1168,9 +1156,12 @@ int dma_async_device_register(struct dma_device *device)
>   	if (rc != 0)
>   		return rc;
>   
> +	mutex_init(&device->chan_mutex);
> +	ida_init(&device->chan_ida);
> +
>   	/* represent channels in sysfs. Probably want devs too */
>   	list_for_each_entry(chan, &device->channels, device_node) {
> -		rc = __dma_async_device_channel_register(device, chan, i++);
> +		rc = __dma_async_device_channel_register(device, chan);
>   		if (rc < 0)
>   			goto err_out;
>   	}
> @@ -1241,6 +1232,7 @@ void dma_async_device_unregister(struct dma_device *device)
>   	 */
>   	dma_cap_set(DMA_PRIVATE, device->cap_mask);
>   	dma_channel_rebalance();
> +	ida_free(&dma_ida, device->dev_id);
>   	dma_device_put(device);
>   	mutex_unlock(&dma_list_mutex);
>   }
> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> index 3a43dbd5f615..e7419038d60f 100644
> --- a/include/linux/dmaengine.h
> +++ b/include/linux/dmaengine.h
> @@ -337,13 +337,11 @@ struct dma_chan {
>    * @chan: driver channel device
>    * @device: sysfs device
>    * @dev_id: parent dma_device dev_id
> - * @idr_ref: reference count to gate release of dma_device dev_id
>    */
>   struct dma_chan_dev {
>   	struct dma_chan *chan;
>   	struct device device;
>   	int dev_id;
> -	atomic_t *idr_ref;
>   };
>   
>   /**
> @@ -846,6 +844,8 @@ struct dma_device {
>   	int dev_id;
>   	struct device *dev;
>   	struct module *owner;
> +	struct ida chan_ida;
> +	struct mutex chan_mutex;	/* to protect chan_ida */
>   
>   	u32 src_addr_widths;
>   	u32 dst_addr_widths;