subject:"\[PATCH 10\/10\] ufs\: fix DMA mask setting"

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

2013-08-15 Thread Akinobu Mita

> On 6/29/2013 11:10 AM, Akinobu Mita wrote:
>> 2013/6/29 James Bottomley :
>>> 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;

This looks the right solution for now.  I'll send revised patches that
include this trick in ufshcd-pltfrm and fixed ufshcd_set_dma_mask() as
James suggested above.

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-08-09 Thread Sujit Reddy Thumma


On 6/29/2013 11:10 AM, Akinobu Mita wrote:

2013/6/29 James Bottomley :

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
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-06-28 Thread Akinobu Mita

2013/6/29 James Bottomley :
> 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.

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-06-28 Thread James Bottomley

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;

James


--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-06-26 Thread Santosh Y

From: Akinobu Mita 

If the controller doesn't support 64-bit addressing mode, it must not
set the DMA mask to 64-bit.  But it's unconditionally trying to set to
64-bit without checking 64-bit addressing support in the controller
capabilities.

It was correctly checked before commit 3b1d05807a9a68c6d0580e9248247a774a4d3be6
("[SCSI] ufs: Segregate PCI Specific Code"), this aims to restore
the correct behaviour.

To achieve this in a generic way, firstly we should push down the DMA
mask setting routine ufshcd_set_dma_mask() from PCI glue driver to core
driver in order to do it for both PCI glue driver and Platform glue
driver.  Secondly, we should change pci_ DMA mapping API to dma_ DMA
mapping API because core driver is independent of glue drivers.

Lastly, we need to relax dma_set_mask(dev, DMA_BIT_MASK(32)) error check
for platform devices on ARM, which do not have a valid dma_mask pointer.

Signed-off-by: Akinobu Mita 
Cc: Vinayak Holikatti 
Cc: "James E.J. Bottomley" 
Cc: linux-scsi@vger.kernel.org
Signed-off-by: Santosh Y 

diff --git a/drivers/scsi/ufs/ufshcd-pci.c b/drivers/scsi/ufs/ufshcd-pci.c
index 48be39a..64d36eb 100644
--- a/drivers/scsi/ufs/ufshcd-pci.c
+++ b/drivers/scsi/ufs/ufshcd-pci.c
@@ -100,26 +100,6 @@ static void ufshcd_pci_remove(struct pci_dev *pdev)
 }
 
 /**
- * ufshcd_set_dma_mask - Set dma mask based on the controller
- *  addressing capability
- * @pdev: PCI device structure
- *
- * Returns 0 for success, non-zero for failure
- */
-static int ufshcd_set_dma_mask(struct pci_dev *pdev)
-{
-   int err;
-
-   if (!pci_set_dma_mask(pdev, DMA_BIT_MASK(64))
-   && !pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(64)))
-   return 0;
-   err = pci_set_dma_mask(pdev, DMA_BIT_MASK(32));
-   if (!err)
-   err = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(32));
-   return err;
-}
-
-/**
  * ufshcd_pci_probe - probe routine of the driver
  * @pdev: pointer to PCI device handle
  * @id: PCI device id
@@ -155,12 +135,6 @@ ufshcd_pci_probe(struct pci_dev *pdev, const struct 
pci_device_id *id)
goto out_release_regions;
}
 
-   err = ufshcd_set_dma_mask(pdev);
-   if (err) {
-   dev_err(&pdev->dev, "set dma mask failed\n");
-   goto out_iounmap;
-   }
-
err = ufshcd_init(&pdev->dev, &hba, mmio_base, pdev->irq);
if (err) {
dev_err(&pdev->dev, "Initialization failed\n");
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
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));
+}
+
+/**
  * ufshcd_init - Driver initialization routine
  * @dev: pointer to device handle
  * @hba_handle: driver private handle
@@ -1761,6 +1780,12 @@ int ufshcd_init(struct device *dev, struct ufs_hba 
**hba_handle,
/* Get Interrupt bit mask per version */
hba->intr_mask = ufshcd_get_intr_mask(hba);
 
+   err = ufshcd_set_dma_mask(hba);
+   if (err) {
+   dev_err(hba->dev, "set dma mask failed\n");
+   goto out_disable;
+   }
+
/* Allocate memory for host memory space */
err = ufshcd_memory_alloc(hba);
if (err) {
-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-06-26 Thread Santosh Y

From: Akinobu Mita 

If the controller doesn't support 64-bit addressing mode, it must not
set the DMA mask to 64-bit.  But it's unconditionally trying to set to
64-bit without checking 64-bit addressing support in the controller
capabilities.

It was correctly checked before commit 3b1d05807a9a68c6d0580e9248247a774a4d3be6
("[SCSI] ufs: Segregate PCI Specific Code"), this aims to restore
the correct behaviour.

To achieve this in a generic way, firstly we should push down the DMA
mask setting routine ufshcd_set_dma_mask() from PCI glue driver to core
driver in order to do it for both PCI glue driver and Platform glue
driver.  Secondly, we should change pci_ DMA mapping API to dma_ DMA
mapping API because core driver is independent of glue drivers.

Lastly, we need to relax dma_set_mask(dev, DMA_BIT_MASK(32)) error check
for platform devices on ARM, which do not have a valid dma_mask pointer.

Signed-off-by: Akinobu Mita 
Cc: Vinayak Holikatti 
Cc: "James E.J. Bottomley" 
Cc: linux-scsi@vger.kernel.org
Signed-off-by: Santosh Y 

diff --git a/drivers/scsi/ufs/ufshcd-pci.c b/drivers/scsi/ufs/ufshcd-pci.c
index 48be39a..64d36eb 100644
--- a/drivers/scsi/ufs/ufshcd-pci.c
+++ b/drivers/scsi/ufs/ufshcd-pci.c
@@ -100,26 +100,6 @@ static void ufshcd_pci_remove(struct pci_dev *pdev)
 }
 
 /**
- * ufshcd_set_dma_mask - Set dma mask based on the controller
- *  addressing capability
- * @pdev: PCI device structure
- *
- * Returns 0 for success, non-zero for failure
- */
-static int ufshcd_set_dma_mask(struct pci_dev *pdev)
-{
-   int err;
-
-   if (!pci_set_dma_mask(pdev, DMA_BIT_MASK(64))
-   && !pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(64)))
-   return 0;
-   err = pci_set_dma_mask(pdev, DMA_BIT_MASK(32));
-   if (!err)
-   err = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(32));
-   return err;
-}
-
-/**
  * ufshcd_pci_probe - probe routine of the driver
  * @pdev: pointer to PCI device handle
  * @id: PCI device id
@@ -155,12 +135,6 @@ ufshcd_pci_probe(struct pci_dev *pdev, const struct 
pci_device_id *id)
goto out_release_regions;
}
 
-   err = ufshcd_set_dma_mask(pdev);
-   if (err) {
-   dev_err(&pdev->dev, "set dma mask failed\n");
-   goto out_iounmap;
-   }
-
err = ufshcd_init(&pdev->dev, &hba, mmio_base, pdev->irq);
if (err) {
dev_err(&pdev->dev, "Initialization failed\n");
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
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));
+}
+
+/**
  * ufshcd_init - Driver initialization routine
  * @dev: pointer to device handle
  * @hba_handle: driver private handle
@@ -1761,6 +1780,12 @@ int ufshcd_init(struct device *dev, struct ufs_hba 
**hba_handle,
/* Get Interrupt bit mask per version */
hba->intr_mask = ufshcd_get_intr_mask(hba);
 
+   err = ufshcd_set_dma_mask(hba);
+   if (err) {
+   dev_err(hba->dev, "set dma mask failed\n");
+   goto out_disable;
+   }
+
/* Allocate memory for host memory space */
err = ufshcd_memory_alloc(hba);
if (err) {
-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

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

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

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

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

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

6 matches

Site Navigation

Mail list logo

Footer information