Re: [PATCH v2 03/12] dmaengine: dw-edma: Add partial channel ownership mode

[Koichiro Den]

On Thu, Jun 04, 2026 at 04:24:21PM -0400, Frank Li wrote:
> On Mon, May 25, 2026 at 03:24:11PM +0900, Koichiro Den wrote:
> > Some endpoint DMA frontends expose only a subset of a controller that is
> > also initialized by the endpoint-side OS. Add a partial ownership flag
> > so dw-edma does not reset controller-wide state in probe() or remove().
> >
> > Keep the mode conservative. Do not enable interrupt-emulation doorbells,
> > and reject partial instances for map formats that this driver cannot safely
> > share. For EDMA_MF_EDMA_UNROLL and EDMA_MF_HDMA_COMPAT, require ownership
> > of all channels in each exposed direction. The driver updates registers
> > shared by all channels in a direction, such as interrupt masks and
> > linked-list error enables, so two independent OS instances cannot safely
> > split one direction without a shared locking protocol, which is
> > unrealistic.
> >
> > The frontend must still quiesce delegated channels before removing a
> > partial instance. The flag only keeps probe() and remove() from
> > resetting controller-wide state that may belong to a peer OS instance.
> >
> > Signed-off-by: Koichiro Den <den@xxxxxxxxxxxxx>
> > ---
> > Changes in v2:
> >   - Reject partial ownership for unsupported map formats up front,
> >     keep direction-granularity validation limited to supported formats.
> >   - Revise the commit message accordingly.
> >
> >  drivers/dma/dw-edma/dw-edma-core.c | 47 +++++++++++++++++++++++-------
> >  include/linux/dma/edma.h           |  6 ++++
> >  2 files changed, 43 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> > index a70e0640d082..fcef9a27b6ce 100644
> > --- a/drivers/dma/dw-edma/dw-edma-core.c
> > +++ b/drivers/dma/dw-edma/dw-edma-core.c
> > @@ -794,6 +794,9 @@ static int dw_edma_emul_irq_alloc(struct dw_edma *dw)
> >  	chip->db_irq = 0;
> >  	chip->db_offset = ~0;
> >
> > +	if (chip->flags & DW_EDMA_CHIP_PARTIAL)
> > +		return 0;
> > +
> >  	/*
> >  	 * Only meaningful when the core provides the deassert sequence
> >  	 * for interrupt emulation.
> > @@ -1135,6 +1138,8 @@ int dw_edma_probe(struct dw_edma_chip *chip)
> >  {
> >  	struct device *dev;
> >  	struct dw_edma *dw;
> > +	u16 hw_wr_ch_cnt;
> > +	u16 hw_rd_ch_cnt;
> >  	u32 wr_alloc = 0;
> >  	u32 rd_alloc = 0;
> >  	int i, err;
> > @@ -1146,6 +1151,16 @@ int dw_edma_probe(struct dw_edma_chip *chip)
> >  	if (!dev || !chip->ops)
> >  		return -EINVAL;
> >
> > +	if (chip->flags & DW_EDMA_CHIP_PARTIAL) {
> > +		switch (chip->mf) {
> > +		case EDMA_MF_EDMA_UNROLL:
> > +		case EDMA_MF_HDMA_COMPAT:
> > +			break;
> > +		default:
> > +			return -EOPNOTSUPP;
> > +		}
> > +	}
> > +
> >  	dw = devm_kzalloc(dev, sizeof(*dw), GFP_KERNEL);
> >  	if (!dw)
> >  		return -ENOMEM;
> > @@ -1159,13 +1174,23 @@ int dw_edma_probe(struct dw_edma_chip *chip)
> >
> >  	raw_spin_lock_init(&dw->lock);
> >
> > -	dw->wr_ch_cnt = min_t(u16, chip->ll_wr_cnt,
> > -			      dw_edma_core_ch_count(dw, EDMA_DIR_WRITE));
> > -	dw->wr_ch_cnt = min_t(u16, dw->wr_ch_cnt, EDMA_MAX_WR_CH);
> > +	hw_wr_ch_cnt = min_t(u16, dw_edma_core_ch_count(dw, EDMA_DIR_WRITE),
> > +			     EDMA_MAX_WR_CH);
> > +	hw_rd_ch_cnt = min_t(u16, dw_edma_core_ch_count(dw, EDMA_DIR_READ),
> > +			     EDMA_MAX_RD_CH);
> > +
> > +	if (chip->flags & DW_EDMA_CHIP_PARTIAL) {
> > +		/*
> > +		 * Direction-wide registers are shared by all channels in that
> > +		 * direction, so a direction must have a single owner.
> > +		 */
> > +		if ((chip->ll_wr_cnt && chip->ll_wr_cnt != hw_wr_ch_cnt) ||
> > +		    (chip->ll_rd_cnt && chip->ll_rd_cnt != hw_rd_ch_cnt))
> > +			return -EOPNOTSUPP;
> > +	}
> >
> > -	dw->rd_ch_cnt = min_t(u16, chip->ll_rd_cnt,
> > -			      dw_edma_core_ch_count(dw, EDMA_DIR_READ));
> > -	dw->rd_ch_cnt = min_t(u16, dw->rd_ch_cnt, EDMA_MAX_RD_CH);
> > +	dw->wr_ch_cnt = min_t(u16, chip->ll_wr_cnt, hw_wr_ch_cnt);
> > +	dw->rd_ch_cnt = min_t(u16, chip->ll_rd_cnt, hw_rd_ch_cnt);
> >
> >  	if (!dw->wr_ch_cnt && !dw->rd_ch_cnt)
> >  		return -EINVAL;
> > @@ -1182,8 +1207,10 @@ int dw_edma_probe(struct dw_edma_chip *chip)
> >  	snprintf(dw->name, sizeof(dw->name), "dw-edma-core:%s",
> >  		 dev_name(chip->dev));
> >
> > -	/* Disable eDMA, only to establish the ideal initial conditions */
> > -	dw_edma_core_off(dw);
> > +	if (!(chip->flags & DW_EDMA_CHIP_PARTIAL)) {
> > +		/* Disable eDMA only when this instance owns the controller. */
> > +		dw_edma_core_off(dw);
> > +	}
> >
> >  	/* Request IRQs */
> >  	err = dw_edma_irq_request(dw, &wr_alloc, &rd_alloc);
> > @@ -1227,8 +1254,8 @@ int dw_edma_remove(struct dw_edma_chip *chip)
> >  	if (!dw)
> >  		return -ENODEV;
> >
> > -	/* Disable eDMA */
> > -	dw_edma_core_off(dw);
> > +	if (!(chip->flags & DW_EDMA_CHIP_PARTIAL))
> > +		dw_edma_core_off(dw);
> 
> Can we simplely prevent dma driver remove? If attached to pci host,
> remove edma driver always be risk because RC may write data at any time.
> 
> And it doesn't make sense to remove EP and EDMA driver after linkup.

Do you mean preventing driver unbind/remove itself regardless of chip->flags,
rather than only avoiding dw_edma_core_off()?

That might be a better direction, but it sounds a broader policy change and I'm
not sure it would not affect existing use cases.

Best regards,
Koichiro

> 
> Frank
> 
> >
> >  	/* Free irqs */
> >  	for (i = (dw->nr_irqs - 1); i >= 0; i--)
> > diff --git a/include/linux/dma/edma.h b/include/linux/dma/edma.h
> > index 2bf2298711e1..84f0e728d300 100644
> > --- a/include/linux/dma/edma.h
> > +++ b/include/linux/dma/edma.h
> > @@ -55,9 +55,15 @@ enum dw_edma_map_format {
> >  /**
> >   * enum dw_edma_chip_flags - Flags specific to an eDMA chip
> >   * @DW_EDMA_CHIP_LOCAL:		eDMA is used locally by an endpoint
> > + * @DW_EDMA_CHIP_PARTIAL:	Only channels described by this instance are
> > + *				owned by this driver. Controller-wide state
> > + *				must be preserved, and layouts with shared
> > + *				direction-wide registers must only be shared at
> > + *				direction granularity.
> >   */
> >  enum dw_edma_chip_flags {
> >  	DW_EDMA_CHIP_LOCAL	= BIT(0),
> > +	DW_EDMA_CHIP_PARTIAL	= BIT(1),
> >  };
> >
> >  /**
> > --
> > 2.51.0
> >