Re: [PATCH] dmaengine: at_xdmac: fix slave configuration issue

Re: [PATCH] dmaengine: at_xdmac: fix slave configuration issue

[Vinod Koul]

On Thu, Apr 16, 2015 at 05:04:00PM +0200, Ludovic Desroches wrote:
> When doing the slave configuration, an error is returned if the maxburst
> value is not supported. The bug comes from the fact that we always check
> the maxburst for both directions but in the case of an unidirectional
> channel, only one is set.
While setting the slave configuration we are not tied to a channel
direction, the direction is passed usin prep_ method. So from that
prespective a channle can be used for tx and rx with same slave config set.

Now if we were invoking at_xdmac_set_slave_config from prep_ calls then it
would have been fine but here we are checking when the slave config is set
so this is not right. You should check maxburst at runtime then...

-- 
~Vinod
> Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
> Cc: <stable@vger.kernel.org> # 3.19 and later
> ---
>  drivers/dma/at_xdmac.c | 97 +++++++++++++++++++++++++++-----------------------
>  1 file changed, 52 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/dma/at_xdmac.c b/drivers/dma/at_xdmac.c
> index d9891d3..30e161b 100644
> --- a/drivers/dma/at_xdmac.c
> +++ b/drivers/dma/at_xdmac.c
> @@ -501,54 +501,61 @@ static int at_xdmac_set_slave_config(struct dma_chan *chan,
>  	u8 dwidth;
>  	int csize;
>  
> -	atchan->cfg[AT_XDMAC_DEV_TO_MEM_CFG] =
> -		AT91_XDMAC_DT_PERID(atchan->perid)
> -		| AT_XDMAC_CC_DAM_INCREMENTED_AM
> -		| AT_XDMAC_CC_SAM_FIXED_AM
> -		| AT_XDMAC_CC_DIF(atchan->memif)
> -		| AT_XDMAC_CC_SIF(atchan->perif)
> -		| AT_XDMAC_CC_SWREQ_HWR_CONNECTED
> -		| AT_XDMAC_CC_DSYNC_PER2MEM
> -		| AT_XDMAC_CC_MBSIZE_SIXTEEN
> -		| AT_XDMAC_CC_TYPE_PER_TRAN;
> -	csize = at_xdmac_csize(sconfig->src_maxburst);
> -	if (csize < 0) {
> -		dev_err(chan2dev(chan), "invalid src maxburst value\n");
> -		return -EINVAL;
> -	}
> -	atchan->cfg[AT_XDMAC_DEV_TO_MEM_CFG] |= AT_XDMAC_CC_CSIZE(csize);
> -	dwidth = ffs(sconfig->src_addr_width) - 1;
> -	atchan->cfg[AT_XDMAC_DEV_TO_MEM_CFG] |= AT_XDMAC_CC_DWIDTH(dwidth);
> -
> -
> -	atchan->cfg[AT_XDMAC_MEM_TO_DEV_CFG] =
> -		AT91_XDMAC_DT_PERID(atchan->perid)
> -		| AT_XDMAC_CC_DAM_FIXED_AM
> -		| AT_XDMAC_CC_SAM_INCREMENTED_AM
> -		| AT_XDMAC_CC_DIF(atchan->perif)
> -		| AT_XDMAC_CC_SIF(atchan->memif)
> -		| AT_XDMAC_CC_SWREQ_HWR_CONNECTED
> -		| AT_XDMAC_CC_DSYNC_MEM2PER
> -		| AT_XDMAC_CC_MBSIZE_SIXTEEN
> -		| AT_XDMAC_CC_TYPE_PER_TRAN;
> -	csize = at_xdmac_csize(sconfig->dst_maxburst);
> -	if (csize < 0) {
> -		dev_err(chan2dev(chan), "invalid src maxburst value\n");
> -		return -EINVAL;
> +	if (sconfig->src_addr) {
> +		atchan->cfg[AT_XDMAC_DEV_TO_MEM_CFG] =
> +			AT91_XDMAC_DT_PERID(atchan->perid)
> +			| AT_XDMAC_CC_DAM_INCREMENTED_AM
> +			| AT_XDMAC_CC_SAM_FIXED_AM
> +			| AT_XDMAC_CC_DIF(atchan->memif)
> +			| AT_XDMAC_CC_SIF(atchan->perif)
> +			| AT_XDMAC_CC_SWREQ_HWR_CONNECTED
> +			| AT_XDMAC_CC_DSYNC_PER2MEM
> +			| AT_XDMAC_CC_MBSIZE_SIXTEEN
> +			| AT_XDMAC_CC_TYPE_PER_TRAN;
> +		csize = at_xdmac_csize(sconfig->src_maxburst);
> +		if (csize < 0) {
> +			dev_err(chan2dev(chan), "invalid src maxburst value\n");
> +			return -EINVAL;
> +		}
> +		atchan->cfg[AT_XDMAC_DEV_TO_MEM_CFG] |= AT_XDMAC_CC_CSIZE(csize);
> +		dwidth = ffs(sconfig->src_addr_width) - 1;
> +		atchan->cfg[AT_XDMAC_DEV_TO_MEM_CFG] |= AT_XDMAC_CC_DWIDTH(dwidth);
> +		/* Src addr is needed to configure the link list descriptor. */
> +		atchan->per_src_addr = sconfig->src_addr;
> +
> +		dev_dbg(chan2dev(chan),
> +			"%s: cfg[dev2mem]=0x%08x, per_src_addr=0x%08x\n",
> +			__func__, atchan->cfg[AT_XDMAC_DEV_TO_MEM_CFG],
> +			atchan->per_src_addr);
>  	}
> -	atchan->cfg[AT_XDMAC_MEM_TO_DEV_CFG] |= AT_XDMAC_CC_CSIZE(csize);
> -	dwidth = ffs(sconfig->dst_addr_width) - 1;
> -	atchan->cfg[AT_XDMAC_MEM_TO_DEV_CFG] |= AT_XDMAC_CC_DWIDTH(dwidth);
>  
> -	/* Src and dst addr are needed to configure the link list descriptor. */
> -	atchan->per_src_addr = sconfig->src_addr;
> -	atchan->per_dst_addr = sconfig->dst_addr;
> +	if (sconfig->dst_addr) {
> +		atchan->cfg[AT_XDMAC_MEM_TO_DEV_CFG] =
> +			AT91_XDMAC_DT_PERID(atchan->perid)
> +			| AT_XDMAC_CC_DAM_FIXED_AM
> +			| AT_XDMAC_CC_SAM_INCREMENTED_AM
> +			| AT_XDMAC_CC_DIF(atchan->perif)
> +			| AT_XDMAC_CC_SIF(atchan->memif)
> +			| AT_XDMAC_CC_SWREQ_HWR_CONNECTED
> +			| AT_XDMAC_CC_DSYNC_MEM2PER
> +			| AT_XDMAC_CC_MBSIZE_SIXTEEN
> +			| AT_XDMAC_CC_TYPE_PER_TRAN;
> +		csize = at_xdmac_csize(sconfig->dst_maxburst);
> +		if (csize < 0) {
> +			dev_err(chan2dev(chan), "invalid src maxburst value\n");
> +			return -EINVAL;
> +		}
> +		atchan->cfg[AT_XDMAC_MEM_TO_DEV_CFG] |= AT_XDMAC_CC_CSIZE(csize);
> +		dwidth = ffs(sconfig->dst_addr_width) - 1;
> +		atchan->cfg[AT_XDMAC_MEM_TO_DEV_CFG] |= AT_XDMAC_CC_DWIDTH(dwidth);
> +		/* Dst addr is needed to configure the link list descriptor. */
> +		atchan->per_dst_addr = sconfig->dst_addr;
>  
> -	dev_dbg(chan2dev(chan),
> -		"%s: cfg[dev2mem]=0x%08x, cfg[mem2dev]=0x%08x, per_src_addr=0x%08x, per_dst_addr=0x%08x\n",
> -		__func__, atchan->cfg[AT_XDMAC_DEV_TO_MEM_CFG],
> -		atchan->cfg[AT_XDMAC_MEM_TO_DEV_CFG],
> -		atchan->per_src_addr, atchan->per_dst_addr);
> +		dev_dbg(chan2dev(chan),
> +			"%s: cfg[mem2dev]=0x%08x, per_dst_addr=0x%08x\n",
> +			__func__, atchan->cfg[AT_XDMAC_MEM_TO_DEV_CFG],
> +			atchan->per_dst_addr);
> +	}
>  
>  	return 0;
>  }
> -- 
> 2.2.0
> 

-- 

Re: [PATCH] dmaengine: at_xdmac: fix slave configuration issue

[Ludovic Desroches]

Hi Vinod,

On Mon, Apr 27, 2015 at 09:03:28AM +0530, Vinod Koul wrote:
> On Thu, Apr 16, 2015 at 05:04:00PM +0200, Ludovic Desroches wrote:
> > When doing the slave configuration, an error is returned if the maxburst
> > value is not supported. The bug comes from the fact that we always check
> > the maxburst for both directions but in the case of an unidirectional
> > channel, only one is set.
> While setting the slave configuration we are not tied to a channel
> direction, the direction is passed usin prep_ method. So from that
> prespective a channle can be used for tx and rx with same slave config set.
> 
> Now if we were invoking at_xdmac_set_slave_config from prep_ calls then it
> would have been fine but here we are checking when the slave config is set
> so this is not right. You should check maxburst at runtime then...
> 

I don't understand why we should wait before checking the
configuration... Some channels are unidirectionnal so implicitly we know
the direction at configuration time because the device will fill only a
part of the dma_slave_config structure. For example, the atmel usart
requests a tx and a rx channels. When configuring the tx channel, only
the dst_ fields of the dma_slave_config structure are filled. Is it a
bad behavior?

The change introduced by this patch doesn't really care about the
direction, it only tells that if the device only fills src_ fields then
I don't have to check fields not configured.

Regards

Ludovic

> -- 
> ~Vinod
> > Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
> > Cc: <stable@vger.kernel.org> # 3.19 and later
> > ---
> >  drivers/dma/at_xdmac.c | 97 +++++++++++++++++++++++++++-----------------------
> >  1 file changed, 52 insertions(+), 45 deletions(-)
> > 
> > diff --git a/drivers/dma/at_xdmac.c b/drivers/dma/at_xdmac.c
> > index d9891d3..30e161b 100644
> > --- a/drivers/dma/at_xdmac.c
> > +++ b/drivers/dma/at_xdmac.c
> > @@ -501,54 +501,61 @@ static int at_xdmac_set_slave_config(struct dma_chan *chan,
> >  	u8 dwidth;
> >  	int csize;
> >  
> > -	atchan->cfg[AT_XDMAC_DEV_TO_MEM_CFG] =
> > -		AT91_XDMAC_DT_PERID(atchan->perid)
> > -		| AT_XDMAC_CC_DAM_INCREMENTED_AM
> > -		| AT_XDMAC_CC_SAM_FIXED_AM
> > -		| AT_XDMAC_CC_DIF(atchan->memif)
> > -		| AT_XDMAC_CC_SIF(atchan->perif)
> > -		| AT_XDMAC_CC_SWREQ_HWR_CONNECTED
> > -		| AT_XDMAC_CC_DSYNC_PER2MEM
> > -		| AT_XDMAC_CC_MBSIZE_SIXTEEN
> > -		| AT_XDMAC_CC_TYPE_PER_TRAN;
> > -	csize = at_xdmac_csize(sconfig->src_maxburst);
> > -	if (csize < 0) {
> > -		dev_err(chan2dev(chan), "invalid src maxburst value\n");
> > -		return -EINVAL;
> > -	}
> > -	atchan->cfg[AT_XDMAC_DEV_TO_MEM_CFG] |= AT_XDMAC_CC_CSIZE(csize);
> > -	dwidth = ffs(sconfig->src_addr_width) - 1;
> > -	atchan->cfg[AT_XDMAC_DEV_TO_MEM_CFG] |= AT_XDMAC_CC_DWIDTH(dwidth);
> > -
> > -
> > -	atchan->cfg[AT_XDMAC_MEM_TO_DEV_CFG] =
> > -		AT91_XDMAC_DT_PERID(atchan->perid)
> > -		| AT_XDMAC_CC_DAM_FIXED_AM
> > -		| AT_XDMAC_CC_SAM_INCREMENTED_AM
> > -		| AT_XDMAC_CC_DIF(atchan->perif)
> > -		| AT_XDMAC_CC_SIF(atchan->memif)
> > -		| AT_XDMAC_CC_SWREQ_HWR_CONNECTED
> > -		| AT_XDMAC_CC_DSYNC_MEM2PER
> > -		| AT_XDMAC_CC_MBSIZE_SIXTEEN
> > -		| AT_XDMAC_CC_TYPE_PER_TRAN;
> > -	csize = at_xdmac_csize(sconfig->dst_maxburst);
> > -	if (csize < 0) {
> > -		dev_err(chan2dev(chan), "invalid src maxburst value\n");
> > -		return -EINVAL;
> > +	if (sconfig->src_addr) {
> > +		atchan->cfg[AT_XDMAC_DEV_TO_MEM_CFG] =
> > +			AT91_XDMAC_DT_PERID(atchan->perid)
> > +			| AT_XDMAC_CC_DAM_INCREMENTED_AM
> > +			| AT_XDMAC_CC_SAM_FIXED_AM
> > +			| AT_XDMAC_CC_DIF(atchan->memif)
> > +			| AT_XDMAC_CC_SIF(atchan->perif)
> > +			| AT_XDMAC_CC_SWREQ_HWR_CONNECTED
> > +			| AT_XDMAC_CC_DSYNC_PER2MEM
> > +			| AT_XDMAC_CC_MBSIZE_SIXTEEN
> > +			| AT_XDMAC_CC_TYPE_PER_TRAN;
> > +		csize = at_xdmac_csize(sconfig->src_maxburst);
> > +		if (csize < 0) {
> > +			dev_err(chan2dev(chan), "invalid src maxburst value\n");
> > +			return -EINVAL;
> > +		}
> > +		atchan->cfg[AT_XDMAC_DEV_TO_MEM_CFG] |= AT_XDMAC_CC_CSIZE(csize);
> > +		dwidth = ffs(sconfig->src_addr_width) - 1;
> > +		atchan->cfg[AT_XDMAC_DEV_TO_MEM_CFG] |= AT_XDMAC_CC_DWIDTH(dwidth);
> > +		/* Src addr is needed to configure the link list descriptor. */
> > +		atchan->per_src_addr = sconfig->src_addr;
> > +
> > +		dev_dbg(chan2dev(chan),
> > +			"%s: cfg[dev2mem]=0x%08x, per_src_addr=0x%08x\n",
> > +			__func__, atchan->cfg[AT_XDMAC_DEV_TO_MEM_CFG],
> > +			atchan->per_src_addr);
> >  	}
> > -	atchan->cfg[AT_XDMAC_MEM_TO_DEV_CFG] |= AT_XDMAC_CC_CSIZE(csize);
> > -	dwidth = ffs(sconfig->dst_addr_width) - 1;
> > -	atchan->cfg[AT_XDMAC_MEM_TO_DEV_CFG] |= AT_XDMAC_CC_DWIDTH(dwidth);
> >  
> > -	/* Src and dst addr are needed to configure the link list descriptor. */
> > -	atchan->per_src_addr = sconfig->src_addr;
> > -	atchan->per_dst_addr = sconfig->dst_addr;
> > +	if (sconfig->dst_addr) {
> > +		atchan->cfg[AT_XDMAC_MEM_TO_DEV_CFG] =
> > +			AT91_XDMAC_DT_PERID(atchan->perid)
> > +			| AT_XDMAC_CC_DAM_FIXED_AM
> > +			| AT_XDMAC_CC_SAM_INCREMENTED_AM
> > +			| AT_XDMAC_CC_DIF(atchan->perif)
> > +			| AT_XDMAC_CC_SIF(atchan->memif)
> > +			| AT_XDMAC_CC_SWREQ_HWR_CONNECTED
> > +			| AT_XDMAC_CC_DSYNC_MEM2PER
> > +			| AT_XDMAC_CC_MBSIZE_SIXTEEN
> > +			| AT_XDMAC_CC_TYPE_PER_TRAN;
> > +		csize = at_xdmac_csize(sconfig->dst_maxburst);
> > +		if (csize < 0) {
> > +			dev_err(chan2dev(chan), "invalid src maxburst value\n");
> > +			return -EINVAL;
> > +		}
> > +		atchan->cfg[AT_XDMAC_MEM_TO_DEV_CFG] |= AT_XDMAC_CC_CSIZE(csize);
> > +		dwidth = ffs(sconfig->dst_addr_width) - 1;
> > +		atchan->cfg[AT_XDMAC_MEM_TO_DEV_CFG] |= AT_XDMAC_CC_DWIDTH(dwidth);
> > +		/* Dst addr is needed to configure the link list descriptor. */
> > +		atchan->per_dst_addr = sconfig->dst_addr;
> >  
> > -	dev_dbg(chan2dev(chan),
> > -		"%s: cfg[dev2mem]=0x%08x, cfg[mem2dev]=0x%08x, per_src_addr=0x%08x, per_dst_addr=0x%08x\n",
> > -		__func__, atchan->cfg[AT_XDMAC_DEV_TO_MEM_CFG],
> > -		atchan->cfg[AT_XDMAC_MEM_TO_DEV_CFG],
> > -		atchan->per_src_addr, atchan->per_dst_addr);
> > +		dev_dbg(chan2dev(chan),
> > +			"%s: cfg[mem2dev]=0x%08x, per_dst_addr=0x%08x\n",
> > +			__func__, atchan->cfg[AT_XDMAC_MEM_TO_DEV_CFG],
> > +			atchan->per_dst_addr);
> > +	}
> >  
> >  	return 0;
> >  }
> > -- 
> > 2.2.0
> > 
> 
> -- 

Re: [PATCH] dmaengine: at_xdmac: fix slave configuration issue

[Vinod Koul]

On Mon, Apr 27, 2015 at 10:33:58AM +0200, Ludovic Desroches wrote:
> Hi Vinod,
> 
> On Mon, Apr 27, 2015 at 09:03:28AM +0530, Vinod Koul wrote:
> > On Thu, Apr 16, 2015 at 05:04:00PM +0200, Ludovic Desroches wrote:
> > > When doing the slave configuration, an error is returned if the maxburst
> > > value is not supported. The bug comes from the fact that we always check
> > > the maxburst for both directions but in the case of an unidirectional
> > > channel, only one is set.
> > While setting the slave configuration we are not tied to a channel
> > direction, the direction is passed usin prep_ method. So from that
> > prespective a channle can be used for tx and rx with same slave config set.
> > 
> > Now if we were invoking at_xdmac_set_slave_config from prep_ calls then it
> > would have been fine but here we are checking when the slave config is set
> > so this is not right. You should check maxburst at runtime then...
> > 
> 
> I don't understand why we should wait before checking the
> configuration... Some channels are unidirectionnal so implicitly we know
> the direction at configuration time because the device will fill only a
> part of the dma_slave_config structure. For example, the atmel usart
> requests a tx and a rx channels. When configuring the tx channel, only
> the dst_ fields of the dma_slave_config structure are filled. Is it a
> bad behavior?
> 
> The change introduced by this patch doesn't really care about the
> direction, it only tells that if the device only fills src_ fields then
> I don't have to check fields not configured.
Well that is because we started with the assumption that channels are
uni-direction and we know that. From client side we shouldn't care
how channel looks like and which dma controller we are talking. The point is
to make clients unaware and use the dmaengine API

-- 
~Vinod

Re: [PATCH] dmaengine: at_xdmac: fix slave configuration issue

[Ludovic Desroches]

On Mon, May 04, 2015 at 01:55:36PM +0530, Vinod Koul wrote:
> On Mon, Apr 27, 2015 at 10:33:58AM +0200, Ludovic Desroches wrote:
> > Hi Vinod,
> > 
> > On Mon, Apr 27, 2015 at 09:03:28AM +0530, Vinod Koul wrote:
> > > On Thu, Apr 16, 2015 at 05:04:00PM +0200, Ludovic Desroches wrote:
> > > > When doing the slave configuration, an error is returned if the maxburst
> > > > value is not supported. The bug comes from the fact that we always check
> > > > the maxburst for both directions but in the case of an unidirectional
> > > > channel, only one is set.
> > > While setting the slave configuration we are not tied to a channel
> > > direction, the direction is passed usin prep_ method. So from that
> > > prespective a channle can be used for tx and rx with same slave config set.
> > > 
> > > Now if we were invoking at_xdmac_set_slave_config from prep_ calls then it
> > > would have been fine but here we are checking when the slave config is set
> > > so this is not right. You should check maxburst at runtime then...
> > > 
> > 
> > I don't understand why we should wait before checking the
> > configuration... Some channels are unidirectionnal so implicitly we know
> > the direction at configuration time because the device will fill only a
> > part of the dma_slave_config structure. For example, the atmel usart
> > requests a tx and a rx channels. When configuring the tx channel, only
> > the dst_ fields of the dma_slave_config structure are filled. Is it a
> > bad behavior?
> > 
> > The change introduced by this patch doesn't really care about the
> > direction, it only tells that if the device only fills src_ fields then
> > I don't have to check fields not configured.
> Well that is because we started with the assumption that channels are
> uni-direction and we know that. From client side we shouldn't care
> how channel looks like and which dma controller we are talking. The point is
> to make clients unaware and use the dmaengine API

I am sorry but I don't understand what is wrong with your vision and
this patch.

I think it is the case you describe, the client doesn't care how the
channel looks like, it wants a channel for doing transmission so it
fills only the configuration part for this purpose. In this case there
is no reason the dma controller returns an error by checking unset
values.

It's true I can check the configuration requested by the client at
runtime. But why waiting so long? If there is an issue, it's better to
return the error as soon as possible. On client side, even at this
moment it is difficult to manage an error. For example, my dma
controller doesn't support the max burst requested, how the client will
know what is the maxburst size it can ask?

Regards

Ludovic

Re: [PATCH] dmaengine: at_xdmac: fix slave configuration issue

[Vinod Koul]

On Mon, May 04, 2015 at 11:12:41AM +0200, Ludovic Desroches wrote:
> On Mon, May 04, 2015 at 01:55:36PM +0530, Vinod Koul wrote:
> > On Mon, Apr 27, 2015 at 10:33:58AM +0200, Ludovic Desroches wrote:
> > > Hi Vinod,
> > > 
> > > On Mon, Apr 27, 2015 at 09:03:28AM +0530, Vinod Koul wrote:
> > > > On Thu, Apr 16, 2015 at 05:04:00PM +0200, Ludovic Desroches wrote:
> > > > > When doing the slave configuration, an error is returned if the maxburst
> > > > > value is not supported. The bug comes from the fact that we always check
> > > > > the maxburst for both directions but in the case of an unidirectional
> > > > > channel, only one is set.
> > > > While setting the slave configuration we are not tied to a channel
> > > > direction, the direction is passed usin prep_ method. So from that
> > > > prespective a channle can be used for tx and rx with same slave config set.
> > > > 
> > > > Now if we were invoking at_xdmac_set_slave_config from prep_ calls then it
> > > > would have been fine but here we are checking when the slave config is set
> > > > so this is not right. You should check maxburst at runtime then...
> > > > 
> > > 
> > > I don't understand why we should wait before checking the
> > > configuration... Some channels are unidirectionnal so implicitly we know
> > > the direction at configuration time because the device will fill only a
> > > part of the dma_slave_config structure. For example, the atmel usart
> > > requests a tx and a rx channels. When configuring the tx channel, only
> > > the dst_ fields of the dma_slave_config structure are filled. Is it a
> > > bad behavior?
> > > 
> > > The change introduced by this patch doesn't really care about the
> > > direction, it only tells that if the device only fills src_ fields then
> > > I don't have to check fields not configured.
> > Well that is because we started with the assumption that channels are
> > uni-direction and we know that. From client side we shouldn't care
> > how channel looks like and which dma controller we are talking. The point is
> > to make clients unaware and use the dmaengine API
> 
> I am sorry but I don't understand what is wrong with your vision and
> this patch.
> 
> I think it is the case you describe, the client doesn't care how the
> channel looks like, it wants a channel for doing transmission so it
> fills only the configuration part for this purpose. In this case there
> is no reason the dma controller returns an error by checking unset
> values.
As i said earlier, if the checks were in prepare call then they would be
fine, but they are in slave_dma_config API. You dont know the direction at
this point so checking here is not good. You should copy the parameters here
as most do and move the checks to prepare_ calls where you know the
direction for argument of prepare and return accordingly.

> 
> It's true I can check the configuration requested by the client at
> runtime. But why waiting so long? If there is an issue, it's better to
> return the error as soon as possible. On client side, even at this
> moment it is difficult to manage an error. For example, my dma
> controller doesn't support the max burst requested, how the client will
> know what is the maxburst size it can ask?
Because you dont have the complete information. And failing prepare at that
time makese sense because parameters have not be set properly!

-- 
~Vinod
> 
> Regards
> 
> Ludovic

-- 

Re: [PATCH] dmaengine: at_xdmac: fix slave configuration issue

[Ludovic Desroches]

On Mon, May 04, 2015 at 04:06:44PM +0530, Vinod Koul wrote:
> On Mon, May 04, 2015 at 11:12:41AM +0200, Ludovic Desroches wrote:
> > On Mon, May 04, 2015 at 01:55:36PM +0530, Vinod Koul wrote:
> > > On Mon, Apr 27, 2015 at 10:33:58AM +0200, Ludovic Desroches wrote:
> > > > Hi Vinod,
> > > > 
> > > > On Mon, Apr 27, 2015 at 09:03:28AM +0530, Vinod Koul wrote:
> > > > > On Thu, Apr 16, 2015 at 05:04:00PM +0200, Ludovic Desroches wrote:
> > > > > > When doing the slave configuration, an error is returned if the maxburst
> > > > > > value is not supported. The bug comes from the fact that we always check
> > > > > > the maxburst for both directions but in the case of an unidirectional
> > > > > > channel, only one is set.
> > > > > While setting the slave configuration we are not tied to a channel
> > > > > direction, the direction is passed usin prep_ method. So from that
> > > > > prespective a channle can be used for tx and rx with same slave config set.
> > > > > 
> > > > > Now if we were invoking at_xdmac_set_slave_config from prep_ calls then it
> > > > > would have been fine but here we are checking when the slave config is set
> > > > > so this is not right. You should check maxburst at runtime then...
> > > > > 
> > > > 
> > > > I don't understand why we should wait before checking the
> > > > configuration... Some channels are unidirectionnal so implicitly we know
> > > > the direction at configuration time because the device will fill only a
> > > > part of the dma_slave_config structure. For example, the atmel usart
> > > > requests a tx and a rx channels. When configuring the tx channel, only
> > > > the dst_ fields of the dma_slave_config structure are filled. Is it a
> > > > bad behavior?
> > > > 
> > > > The change introduced by this patch doesn't really care about the
> > > > direction, it only tells that if the device only fills src_ fields then
> > > > I don't have to check fields not configured.
> > > Well that is because we started with the assumption that channels are
> > > uni-direction and we know that. From client side we shouldn't care
> > > how channel looks like and which dma controller we are talking. The point is
> > > to make clients unaware and use the dmaengine API
> > 
> > I am sorry but I don't understand what is wrong with your vision and
> > this patch.
> > 
> > I think it is the case you describe, the client doesn't care how the
> > channel looks like, it wants a channel for doing transmission so it
> > fills only the configuration part for this purpose. In this case there
> > is no reason the dma controller returns an error by checking unset
> > values.
> As i said earlier, if the checks were in prepare call then they would be
> fine, but they are in slave_dma_config API. You dont know the direction at
> this point so checking here is not good. You should copy the parameters here
> as most do and move the checks to prepare_ calls where you know the
> direction for argument of prepare and return accordingly.
> 
> > 
> > It's true I can check the configuration requested by the client at
> > runtime. But why waiting so long? If there is an issue, it's better to
> > return the error as soon as possible. On client side, even at this
> > moment it is difficult to manage an error. For example, my dma
> > controller doesn't support the max burst requested, how the client will
> > know what is the maxburst size it can ask?
> Because you dont have the complete information. And failing prepare at that
> time makese sense because parameters have not be set properly!
> 

Sorry but I still don't understand why I don't have the complete
information. The only thing missing is the direction and I don't care
about it.

I was updating my patch but I realize I have no benefit to do it in
prep. Worse if I simply copy the slave configuration and check it in prep, I'll
check the maxburst value several times instead of one and it will always be
the same...


Ludovic

Re: [PATCH] dmaengine: at_xdmac: fix slave configuration issue

[Vinod Koul]

On Mon, May 04, 2015 at 04:58:25PM +0200, Ludovic Desroches wrote:
> On Mon, May 04, 2015 at 04:06:44PM +0530, Vinod Koul wrote:
> > On Mon, May 04, 2015 at 11:12:41AM +0200, Ludovic Desroches wrote:
> > > On Mon, May 04, 2015 at 01:55:36PM +0530, Vinod Koul wrote:
> > > > On Mon, Apr 27, 2015 at 10:33:58AM +0200, Ludovic Desroches wrote:
> > > > > Hi Vinod,
> > > > > 
> > > > > On Mon, Apr 27, 2015 at 09:03:28AM +0530, Vinod Koul wrote:
> > > > > > On Thu, Apr 16, 2015 at 05:04:00PM +0200, Ludovic Desroches wrote:
> > > > > > > When doing the slave configuration, an error is returned if the maxburst
> > > > > > > value is not supported. The bug comes from the fact that we always check
> > > > > > > the maxburst for both directions but in the case of an unidirectional
> > > > > > > channel, only one is set.
> > > > > > While setting the slave configuration we are not tied to a channel
> > > > > > direction, the direction is passed usin prep_ method. So from that
> > > > > > prespective a channle can be used for tx and rx with same slave config set.
> > > > > > 
> > > > > > Now if we were invoking at_xdmac_set_slave_config from prep_ calls then it
> > > > > > would have been fine but here we are checking when the slave config is set
> > > > > > so this is not right. You should check maxburst at runtime then...
> > > > > > 
> > > > > 
> > > > > I don't understand why we should wait before checking the
> > > > > configuration... Some channels are unidirectionnal so implicitly we know
> > > > > the direction at configuration time because the device will fill only a
> > > > > part of the dma_slave_config structure. For example, the atmel usart
> > > > > requests a tx and a rx channels. When configuring the tx channel, only
> > > > > the dst_ fields of the dma_slave_config structure are filled. Is it a
> > > > > bad behavior?
> > > > > 
> > > > > The change introduced by this patch doesn't really care about the
> > > > > direction, it only tells that if the device only fills src_ fields then
> > > > > I don't have to check fields not configured.
> > > > Well that is because we started with the assumption that channels are
> > > > uni-direction and we know that. From client side we shouldn't care
> > > > how channel looks like and which dma controller we are talking. The point is
> > > > to make clients unaware and use the dmaengine API
> > > 
> > > I am sorry but I don't understand what is wrong with your vision and
> > > this patch.
> > > 
> > > I think it is the case you describe, the client doesn't care how the
> > > channel looks like, it wants a channel for doing transmission so it
> > > fills only the configuration part for this purpose. In this case there
> > > is no reason the dma controller returns an error by checking unset
> > > values.
> > As i said earlier, if the checks were in prepare call then they would be
> > fine, but they are in slave_dma_config API. You dont know the direction at
> > this point so checking here is not good. You should copy the parameters here
> > as most do and move the checks to prepare_ calls where you know the
> > direction for argument of prepare and return accordingly.
> > 
> > > 
> > > It's true I can check the configuration requested by the client at
> > > runtime. But why waiting so long? If there is an issue, it's better to
> > > return the error as soon as possible. On client side, even at this
> > > moment it is difficult to manage an error. For example, my dma
> > > controller doesn't support the max burst requested, how the client will
> > > know what is the maxburst size it can ask?
> > Because you dont have the complete information. And failing prepare at that
> > time makese sense because parameters have not be set properly!
> > 
> 
> Sorry but I still don't understand why I don't have the complete
> information. The only thing missing is the direction and I don't care
> about it.
> 
> I was updating my patch but I realize I have no benefit to do it in
> prep. Worse if I simply copy the slave configuration and check it in prep, I'll
> check the maxburst value several times instead of one and it will always be
> the same...
ah yes but then you can use the channel for either direction without setting
slave_dma_config...

-- 
~Vinod

Re: [PATCH] dmaengine: at_xdmac: fix slave configuration issue

[Ludovic Desroches]

On Wed, May 06, 2015 at 09:03:43AM +0530, Vinod Koul wrote:
> On Mon, May 04, 2015 at 04:58:25PM +0200, Ludovic Desroches wrote:
> > On Mon, May 04, 2015 at 04:06:44PM +0530, Vinod Koul wrote:
> > > On Mon, May 04, 2015 at 11:12:41AM +0200, Ludovic Desroches wrote:
> > > > On Mon, May 04, 2015 at 01:55:36PM +0530, Vinod Koul wrote:
> > > > > On Mon, Apr 27, 2015 at 10:33:58AM +0200, Ludovic Desroches wrote:
> > > > > > Hi Vinod,
> > > > > > 
> > > > > > On Mon, Apr 27, 2015 at 09:03:28AM +0530, Vinod Koul wrote:
> > > > > > > On Thu, Apr 16, 2015 at 05:04:00PM +0200, Ludovic Desroches wrote:
> > > > > > > > When doing the slave configuration, an error is returned if the maxburst
> > > > > > > > value is not supported. The bug comes from the fact that we always check
> > > > > > > > the maxburst for both directions but in the case of an unidirectional
> > > > > > > > channel, only one is set.
> > > > > > > While setting the slave configuration we are not tied to a channel
> > > > > > > direction, the direction is passed usin prep_ method. So from that
> > > > > > > prespective a channle can be used for tx and rx with same slave config set.
> > > > > > > 
> > > > > > > Now if we were invoking at_xdmac_set_slave_config from prep_ calls then it
> > > > > > > would have been fine but here we are checking when the slave config is set
> > > > > > > so this is not right. You should check maxburst at runtime then...
> > > > > > > 
> > > > > > 
> > > > > > I don't understand why we should wait before checking the
> > > > > > configuration... Some channels are unidirectionnal so implicitly we know
> > > > > > the direction at configuration time because the device will fill only a
> > > > > > part of the dma_slave_config structure. For example, the atmel usart
> > > > > > requests a tx and a rx channels. When configuring the tx channel, only
> > > > > > the dst_ fields of the dma_slave_config structure are filled. Is it a
> > > > > > bad behavior?
> > > > > > 
> > > > > > The change introduced by this patch doesn't really care about the
> > > > > > direction, it only tells that if the device only fills src_ fields then
> > > > > > I don't have to check fields not configured.
> > > > > Well that is because we started with the assumption that channels are
> > > > > uni-direction and we know that. From client side we shouldn't care
> > > > > how channel looks like and which dma controller we are talking. The point is
> > > > > to make clients unaware and use the dmaengine API
> > > > 
> > > > I am sorry but I don't understand what is wrong with your vision and
> > > > this patch.
> > > > 
> > > > I think it is the case you describe, the client doesn't care how the
> > > > channel looks like, it wants a channel for doing transmission so it
> > > > fills only the configuration part for this purpose. In this case there
> > > > is no reason the dma controller returns an error by checking unset
> > > > values.
> > > As i said earlier, if the checks were in prepare call then they would be
> > > fine, but they are in slave_dma_config API. You dont know the direction at
> > > this point so checking here is not good. You should copy the parameters here
> > > as most do and move the checks to prepare_ calls where you know the
> > > direction for argument of prepare and return accordingly.
> > > 
> > > > 
> > > > It's true I can check the configuration requested by the client at
> > > > runtime. But why waiting so long? If there is an issue, it's better to
> > > > return the error as soon as possible. On client side, even at this
> > > > moment it is difficult to manage an error. For example, my dma
> > > > controller doesn't support the max burst requested, how the client will
> > > > know what is the maxburst size it can ask?
> > > Because you dont have the complete information. And failing prepare at that
> > > time makese sense because parameters have not be set properly!
> > > 
> > 
> > Sorry but I still don't understand why I don't have the complete
> > information. The only thing missing is the direction and I don't care
> > about it.
> > 
> > I was updating my patch but I realize I have no benefit to do it in
> > prep. Worse if I simply copy the slave configuration and check it in prep, I'll
> > check the maxburst value several times instead of one and it will always be
> > the same...
> ah yes but then you can use the channel for either direction without setting
> slave_dma_config...
> 

I still need dma_slave_config. In my device_config function, I don't
copy the dma_slave_config but I take information from it to compute a
dev2mem and mem2dev configuration. Then I will select the right one in
the prep function. I think it's quite close from that you want.

In the device_config, I convert the maxburst value to a comprehensive
one for my controller. If it is not a supported one then I'll return an
error?

Are you okay with this procedure? If not, I have totally misunderstood
what you want...

My concern is that a device can fill only what it needs in the
dma_slave_config structure. When doing the configuration, the controller
doesn't care about the direction as you asked but if a maxburst
configuration is not set, I will have a zero value which is not a valid
one. I will return an error but is not...


Ludovic

Re: [PATCH] dmaengine: at_xdmac: fix slave configuration issue

[Vinod Koul]

On Wed, May 06, 2015 at 10:13:42AM +0200, Ludovic Desroches wrote:
> > ah yes but then you can use the channel for either direction without setting
> > slave_dma_config...
> > 
> 
> I still need dma_slave_config. In my device_config function, I don't
> copy the dma_slave_config but I take information from it to compute a
> dev2mem and mem2dev configuration. Then I will select the right one in
> the prep function. I think it's quite close from that you want.
since you do not know the direction here, you dont know how to compute
dev2mem or mem2dev configurations

> 
> In the device_config, I convert the maxburst value to a comprehensive
> one for my controller. If it is not a supported one then I'll return an
> error?
thats fine as long as direction is not used, implicitly as well

> Are you okay with this procedure? If not, I have totally misunderstood
> what you want...
> 
> My concern is that a device can fill only what it needs in the
> dma_slave_config structure. When doing the configuration, the controller
> doesn't care about the direction as you asked but if a maxburst
> configuration is not set, I will have a zero value which is not a valid
> one. I will return an error but is not...
Yes that is why many drivers will save configuration. Checking values
for sanity makes sense. No point in accepting config for 64 max burst if
device support upto 32!

Then you use direction is prepare_ api to compute parameters and if
you detect further errors in configuration for said txn then you are valid
to return error. Again if client has sent one direction parameters and in
prepare you will not return error as direction will be apt so current issue
will be fixed as well

-- 
~Vinod