Re: [PATCH 10/10] ufs: fix DMA mask setting

[Sujit Reddy Thumma <sthumma@codeaurora.org>]

On 6/29/2013 11:10 AM, Akinobu Mita wrote:
> 2013/6/29 James Bottomley <James.Bottomley@hansenpartnership.com>:
>> On Wed, 2013-06-26 at 22:39 +0530, Santosh Y wrote:
>>> index 19618c6..431ddb2 100644
>>> --- a/drivers/scsi/ufs/ufshcd.c
>>> +++ b/drivers/scsi/ufs/ufshcd.c
>>> @@ -1711,6 +1711,25 @@ void ufshcd_remove(struct ufs_hba *hba)
>>>   EXPORT_SYMBOL_GPL(ufshcd_remove);
>>>
>>>   /**
>>> + * ufshcd_set_dma_mask - Set dma mask based on the controller
>>> + *                    addressing capability
>>> + * @hba: per adapter instance
>>> + *
>>> + * Returns 0 for success, non-zero for failure
>>> + */
>>> +static int ufshcd_set_dma_mask(struct ufs_hba *hba)
>>> +{
>>> +     if (hba->capabilities & MASK_64_ADDRESSING_SUPPORT) {
>>> +             if (!dma_set_mask(hba->dev, DMA_BIT_MASK(64)) &&
>>> +                 !dma_set_coherent_mask(hba->dev, DMA_BIT_MASK(64)))
>>> +                     return 0;
>>> +     }
>>> +     dma_set_mask(hba->dev, DMA_BIT_MASK(32));
>>> +
>>> +     return dma_set_coherent_mask(hba->dev, DMA_BIT_MASK(32));
>>> +}
>>
>> This isn't right per the API spec.  The guarantee is that if
>> dma_set_mask() succeeds then dma_set_coherent_mask of the same mask will
>> succeed,  so this should read
>>
>>          int err;
>>
>>          if (hba->capabilities & MASK_64_ADDRESSING_SUPPORT) {
>>                  if (!dma_set_mask(hba->dev, DMA_BIT_MASK(64))) {
>>                          dma_set_coherent_mask(hba->dev, DMA_BIT_MASK(64)))
>>                          return 0;
>>                  }
>>          }
>>          err = dma_set_mask(hba->dev, DMA_BIT_MASK(32));
>>          if (!err)
>>                  dma_set_coherent_mask(hba->dev, DMA_BIT_MASK(32));
>>          return err;
>
> Thanks for the explanation.  I agree that this is the correct definision
> of ufshcd_set_dma_mask().
>
> The reason that I omitted the error check on dma_set_mask(DMA_BIT_MASK(32))
> in the patch was that I was seeing that error due to the luck of
> valid dev->dma_mask pointer on OF platform devices although
> dma_supported(DMA_BIT_MASK(32)) returns true.

The popular trick implemented for device-tree probed devices is -
dev->dma_mask = &dev->coherent_dma_mask;

If you don't agree with this you can have something like -
dev->dma_mask = devm_kzalloc(dev, sizeof(*dev->dma_mask), GFP_KERNEL);
See platform_device_register_full();

>
> However, now I think that I should investigate more about dev->dma_mask
> issue rather than jumping into short-circuit fixing in the driver.  And
> then I will revisit this.


-- 
Regards,
Sujit