Re: [PATCH v2 1/4] dmaengine: add per-channel sysfs attribute groups via chan_groups

[Logan Gunthorpe]

Thanks for the feedback!

On 2026-08-14 14:07, Frank Li wrote:
> New dma API should start from dmaengine

Will do -- when I send v3 (probably after the next merge window).

> Any one outside dmaengine need access dma_list_mutex? I'd like use
> dma_list_mutex directly.

This does need to be exported so it may be used in both ioat and
switchtec-dma. It's used indirectly via the dma_chan_from_dev class
helper so it's use may not have been entirely obvious.

>> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h>>
index b3d251c9734e..f4e7861b2507 100644
>> --- a/include/linux/dmaengine.h
>> +++ b/include/linux/dmaengine.h
>> @@ -803,6 +803,9 @@ struct dma_filter {
>>   * @dev: struct device reference for dma mapping api
>>   * @owner: owner module (automatically set based on the provided dev)
>>   * @chan_ida: unique channel ID
>> + * @chan_groups: optional NULL-terminated array of sysfs attribute groups
>> + *	added to each channel's struct device, for driver-specific per-channel
>> + *	attributes
>>   * @src_addr_widths: bit mask of src addr widths the device supports
>>   *	Width is specified in bytes, e.g. for a device supporting
>>   *	a width of 4 the mask should have BIT(4) set.
>> @@ -886,6 +889,7 @@ struct dma_device {
>>  	struct device *dev;
>>  	struct module *owner;
>>  	struct ida chan_ida;
>> +	const struct attribute_group **chan_groups;
> 
> It is per channel group, why not put into dma_chan struct.

I'd like to push back on this lightly. While it may be a per-channel
group, each channel would almost certainly be the same in all cases. So
it's wasting a pointer for every channel that will always be the same.
And the switchtec driver will need to add a new for-loop to set them
all. A snippet of a patch below shows how this would affect both
drivers. I'd personally perfer to leave it, but if you still think it's
better the other way please let me know and I'll change it for v3.

Thanks,

Logan



--- a/drivers/dma/ioat/init.c
+++ b/drivers/dma/ioat/init.c
@@ -1164,6 +1164,7 @@ static int ioat3_dma_probe(struct ioatdma_device
*ioat_dma, int dca)
      list_for_each_entry(c, &dma->channels, device_node) {
              ioat_chan = to_ioat_chan(c);
+             c->chan_groups = ioat_groups;
              writel(IOAT_DMA_DCA_ANY_CPU,
                     ioat_chan->reg_base + IOAT_DCACTRL_OFFSET);
      }
@@ -1183,7 +1184,6 @@ static int ioat3_dma_probe(struct ioatdma_device
*ioat_dma, int dca)
              goto err_disable_interrupts;
      }

-     dma->chan_groups = ioat_groups;
      err = dma_async_device_register(&ioat_dma->dma_dev);
      if (err)
              goto err_disable_interrupts;

--- a/drivers/dma/switchtec_dma.c
+++ b/drivers/dma/switchtec_dma.c
@@ -1769,7 +1769,9 @@ static int switchtec_dma_create(struct pci_dev
*pdev, ...)
      dma->device_synchronize = switchtec_dma_synchronize;
      dma->device_release = switchtec_dma_release;
-     dma->chan_groups = switchtec_groups;
+
+     list_for_each_entry(chan, &dma->channels, device_node)
+             chan->chan_groups = switchtec_groups;

      rc = dma_async_device_register(dma);
      if (rc) {