Re: [PATCH 1/2] spi: atmel: convert to dma_request_slave_channel_compat()

[Richard Genoud]

2013/6/3 Ludovic Desroches <ludovic.desroches@xxxxxxxxx>:
> On Fri, May 31, 2013 at 05:01:59PM +0200, Richard Genoud wrote:
>> Use generic DMA DT helper.
>> Platforms booting with or without DT populated are both supported.
>>
>> Based on Ludovic Desroches <ludovic.desroches@xxxxxxxxx> patchset
>> "ARM: at91: move to generic DMA device tree binding"
>>
>> Signed-off-by: Richard Genoud <richard.genoud@xxxxxxxxx>
>
> Looks fine for me to so
> Acked-by: Ludovic Desroches <ludovic.desroches@xxxxxxxxx>
>
> One comment below
>
Response below.
>> ---
>>  drivers/spi/spi-atmel.c |   42 +++++++++++++++++++++++++++---------------
>>  1 file changed, 27 insertions(+), 15 deletions(-)
>>
>> rebased on linux-next next-20130531
>>
>> diff --git a/drivers/spi/spi-atmel.c b/drivers/spi/spi-atmel.c
>> index 31cfc87..ea1ec00 100644
>> --- a/drivers/spi/spi-atmel.c
>> +++ b/drivers/spi/spi-atmel.c
>> @@ -424,10 +424,15 @@ static int atmel_spi_dma_slave_config(struct atmel_spi *as,
>>       return err;
>>  }
>>
>> -static bool filter(struct dma_chan *chan, void *slave)
>> +static bool filter(struct dma_chan *chan, void *pdata)
>>  {
>> -     struct  at_dma_slave *sl = slave;
>> +     struct atmel_spi_dma *sl_pdata = pdata;
>> +     struct at_dma_slave *sl;
>>
>> +     if (!sl_pdata)
>> +             return false;
>> +
>> +     sl = &sl_pdata->dma_slave;
>>       if (sl->dma_dev == chan->device->dev) {
>
> I am wondering if a null pointer dereference can happen here. In
> atmel_spi_configure_dma sdata was checked so maybe sl should be check
> here.

sdata was checked, but there was no point checking it:
if as is not null, as->dma.dma_slave is defined ; so its address is not null.
it's the same in the filter function now:
if sl_pdata is not null, sl_pdata->dma_slave is defined ; so its
address is not null.

I added a check on sl_pdata in this patch, maybe a (BUG|WARN)_ON()
would have been better, as it really should not happen.


>>               chan->private = sl;
>>               return true;
>> @@ -438,24 +443,31 @@ static bool filter(struct dma_chan *chan, void *slave)
>>
>>  static int atmel_spi_configure_dma(struct atmel_spi *as)
>>  {
>> -     struct at_dma_slave *sdata = &as->dma.dma_slave;
>>       struct dma_slave_config slave_config;
>> +     struct device *dev = &as->pdev->dev;
>>       int err;
>>
>> -     if (sdata && sdata->dma_dev) {
>> -             dma_cap_mask_t mask;
>> +     dma_cap_mask_t mask;
>> +     dma_cap_zero(mask);
>> +     dma_cap_set(DMA_SLAVE, mask);
>>
>> -             /* Try to grab two DMA channels */
>> -             dma_cap_zero(mask);
>> -             dma_cap_set(DMA_SLAVE, mask);
>> -             as->dma.chan_tx = dma_request_channel(mask, filter, sdata);
>> -             if (as->dma.chan_tx)
>> -                     as->dma.chan_rx =
>> -                             dma_request_channel(mask, filter, sdata);
>> +     as->dma.chan_tx = dma_request_slave_channel_compat(mask, filter,
>> +                                                        &as->dma,
>> +                                                        dev, "tx");
>> +     if (!as->dma.chan_tx) {
>> +             dev_err(dev,
>> +                     "DMA TX channel not available, SPI unable to use DMA\n");
>> +             err = -EBUSY;
>> +             goto error;
>>       }
>> -     if (!as->dma.chan_rx || !as->dma.chan_tx) {
>> -             dev_err(&as->pdev->dev,
>> -                     "DMA channel not available, SPI unable to use DMA\n");
>> +
>> +     as->dma.chan_rx = dma_request_slave_channel_compat(mask, filter,
>> +                                                        &as->dma,
>> +                                                        dev, "rx");
>> +
>> +     if (!as->dma.chan_rx) {
>> +             dev_err(dev,
>> +                     "DMA RX channel not available, SPI unable to use DMA\n");
>>               err = -EBUSY;
>>               goto error;
>>       }
>> --
>> 1.7.10.4
>>

Best regards,

Richard.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/