Re: [PATCH v2 01/13] iommu/rockchip: Request irqs in rk_iommu_probe()

2018-01-17 Thread JeffyChen 

Hi Robin,

On 01/17/2018 08:18 PM, Robin Murphy wrote:


@@ -91,7 +92,6 @@ struct rk_iommu {
  void __iomem **bases;
  int num_mmu;
  int *irq;


Nit: irq seems to be redundant now as well.

oops, will fix it.



-int num_irq;
  bool reset_disabled;
  struct iommu_device iommu;
  struct list_head node; /* entry in rk_iommu_domain.iommus */
@@ -830,13 +830,6 @@ static int rk_iommu_attach_device(struct
iommu_domain *domain,
  iommu->domain = domain;
-for (i = 0; i < iommu->num_irq; i++) {
-ret = devm_request_irq(iommu->dev, iommu->irq[i], rk_iommu_irq,
-   IRQF_SHARED, dev_name(dev), iommu);
-if (ret)
-return ret;
-}
-
  for (i = 0; i < iommu->num_mmu; i++) {
  rk_iommu_write(iommu->bases[i], RK_MMU_DTE_ADDR,
 rk_domain->dt_dma);
@@ -885,9 +878,6 @@ static void rk_iommu_detach_device(struct
iommu_domain *domain,
  }
  rk_iommu_disable_stall(iommu);
-for (i = 0; i < iommu->num_irq; i++)
-devm_free_irq(iommu->dev, iommu->irq[i], iommu);
-
  iommu->domain = NULL;
  dev_dbg(dev, "Detached from iommu domain\n");
@@ -1138,7 +1128,7 @@ static int rk_iommu_probe(struct platform_device
*pdev)
  struct rk_iommu *iommu;
  struct resource *res;
  int num_res = pdev->num_resources;
-int err, i;
+int err, i, irq, num_irq;
  iommu = devm_kzalloc(dev, sizeof(*iommu), GFP_KERNEL);
  if (!iommu)
@@ -1165,23 +1155,17 @@ static int rk_iommu_probe(struct
platform_device *pdev)
  if (iommu->num_mmu == 0)
  return PTR_ERR(iommu->bases[0]);
-iommu->num_irq = platform_irq_count(pdev);
-if (iommu->num_irq < 0)
-return iommu->num_irq;
-if (iommu->num_irq == 0)
-return -ENXIO;
-
-iommu->irq = devm_kcalloc(dev, iommu->num_irq, sizeof(*iommu->irq),
-  GFP_KERNEL);
-if (!iommu->irq)
-return -ENOMEM;
-
-for (i = 0; i < iommu->num_irq; i++) {
-iommu->irq[i] = platform_get_irq(pdev, i);
-if (iommu->irq[i] < 0) {
-dev_err(dev, "Failed to get IRQ, %d\n", iommu->irq[i]);
+num_irq = of_irq_count(dev->of_node);


To follow up on the other reply, I'm not sure you really need to count
the IRQs beforehand at all - you're going to be looping through
platform_get_irq() and handling errors anyway, so you may as well just
start at 0 and keep going until -ENOENT (or use platform_get_resource()
to double-check whether an index should be valid, as we do in arm_smmu).

ok, will do that.


Otherwise, it looks like everything that the IRQ handler needs in the
iommu struct (dev, num_mmu and bases) is already initialised by this
point, so we should be OK with respect to races.

ok.


Robin.

Re: [PATCH v2 01/13] iommu/rockchip: Request irqs in rk_iommu_probe()

2018-01-17 Thread JeffyChen 

Hi Robin,

On 01/17/2018 08:18 PM, Robin Murphy wrote:


@@ -91,7 +92,6 @@ struct rk_iommu {
  void __iomem **bases;
  int num_mmu;
  int *irq;


Nit: irq seems to be redundant now as well.

oops, will fix it.



-int num_irq;
  bool reset_disabled;
  struct iommu_device iommu;
  struct list_head node; /* entry in rk_iommu_domain.iommus */
@@ -830,13 +830,6 @@ static int rk_iommu_attach_device(struct
iommu_domain *domain,
  iommu->domain = domain;
-for (i = 0; i < iommu->num_irq; i++) {
-ret = devm_request_irq(iommu->dev, iommu->irq[i], rk_iommu_irq,
-   IRQF_SHARED, dev_name(dev), iommu);
-if (ret)
-return ret;
-}
-
  for (i = 0; i < iommu->num_mmu; i++) {
  rk_iommu_write(iommu->bases[i], RK_MMU_DTE_ADDR,
 rk_domain->dt_dma);
@@ -885,9 +878,6 @@ static void rk_iommu_detach_device(struct
iommu_domain *domain,
  }
  rk_iommu_disable_stall(iommu);
-for (i = 0; i < iommu->num_irq; i++)
-devm_free_irq(iommu->dev, iommu->irq[i], iommu);
-
  iommu->domain = NULL;
  dev_dbg(dev, "Detached from iommu domain\n");
@@ -1138,7 +1128,7 @@ static int rk_iommu_probe(struct platform_device
*pdev)
  struct rk_iommu *iommu;
  struct resource *res;
  int num_res = pdev->num_resources;
-int err, i;
+int err, i, irq, num_irq;
  iommu = devm_kzalloc(dev, sizeof(*iommu), GFP_KERNEL);
  if (!iommu)
@@ -1165,23 +1155,17 @@ static int rk_iommu_probe(struct
platform_device *pdev)
  if (iommu->num_mmu == 0)
  return PTR_ERR(iommu->bases[0]);
-iommu->num_irq = platform_irq_count(pdev);
-if (iommu->num_irq < 0)
-return iommu->num_irq;
-if (iommu->num_irq == 0)
-return -ENXIO;
-
-iommu->irq = devm_kcalloc(dev, iommu->num_irq, sizeof(*iommu->irq),
-  GFP_KERNEL);
-if (!iommu->irq)
-return -ENOMEM;
-
-for (i = 0; i < iommu->num_irq; i++) {
-iommu->irq[i] = platform_get_irq(pdev, i);
-if (iommu->irq[i] < 0) {
-dev_err(dev, "Failed to get IRQ, %d\n", iommu->irq[i]);
+num_irq = of_irq_count(dev->of_node);


To follow up on the other reply, I'm not sure you really need to count
the IRQs beforehand at all - you're going to be looping through
platform_get_irq() and handling errors anyway, so you may as well just
start at 0 and keep going until -ENOENT (or use platform_get_resource()
to double-check whether an index should be valid, as we do in arm_smmu).

ok, will do that.


Otherwise, it looks like everything that the IRQ handler needs in the
iommu struct (dev, num_mmu and bases) is already initialised by this
point, so we should be OK with respect to races.

ok.


Robin.

Re: [PATCH v2 01/13] iommu/rockchip: Request irqs in rk_iommu_probe()

2018-01-17 Thread Robin Murphy 

On 16/01/18 13:25, Jeffy Chen wrote:

Suggested-by: Robin Murphy 
Signed-off-by: Jeffy Chen 
---

Changes in v2: None

  drivers/iommu/rockchip-iommu.c | 38 +++---
  1 file changed, 11 insertions(+), 27 deletions(-)

diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
index 9d991c2d8767..4a12d4746095 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -18,6 +18,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  #include 
  #include 
@@ -91,7 +92,6 @@ struct rk_iommu {
void __iomem **bases;
int num_mmu;
int *irq;


Nit: irq seems to be redundant now as well.


-   int num_irq;
bool reset_disabled;
struct iommu_device iommu;
struct list_head node; /* entry in rk_iommu_domain.iommus */
@@ -830,13 +830,6 @@ static int rk_iommu_attach_device(struct iommu_domain 
*domain,
  
  	iommu->domain = domain;
  
-	for (i = 0; i < iommu->num_irq; i++) {

-   ret = devm_request_irq(iommu->dev, iommu->irq[i], rk_iommu_irq,
-  IRQF_SHARED, dev_name(dev), iommu);
-   if (ret)
-   return ret;
-   }
-
for (i = 0; i < iommu->num_mmu; i++) {
rk_iommu_write(iommu->bases[i], RK_MMU_DTE_ADDR,
   rk_domain->dt_dma);
@@ -885,9 +878,6 @@ static void rk_iommu_detach_device(struct iommu_domain 
*domain,
}
rk_iommu_disable_stall(iommu);
  
-	for (i = 0; i < iommu->num_irq; i++)

-   devm_free_irq(iommu->dev, iommu->irq[i], iommu);
-
iommu->domain = NULL;
  
  	dev_dbg(dev, "Detached from iommu domain\n");

@@ -1138,7 +1128,7 @@ static int rk_iommu_probe(struct platform_device *pdev)
struct rk_iommu *iommu;
struct resource *res;
int num_res = pdev->num_resources;
-   int err, i;
+   int err, i, irq, num_irq;
  
  	iommu = devm_kzalloc(dev, sizeof(*iommu), GFP_KERNEL);

if (!iommu)
@@ -1165,23 +1155,17 @@ static int rk_iommu_probe(struct platform_device *pdev)
if (iommu->num_mmu == 0)
return PTR_ERR(iommu->bases[0]);
  
-	iommu->num_irq = platform_irq_count(pdev);

-   if (iommu->num_irq < 0)
-   return iommu->num_irq;
-   if (iommu->num_irq == 0)
-   return -ENXIO;
-
-   iommu->irq = devm_kcalloc(dev, iommu->num_irq, sizeof(*iommu->irq),
- GFP_KERNEL);
-   if (!iommu->irq)
-   return -ENOMEM;
-
-   for (i = 0; i < iommu->num_irq; i++) {
-   iommu->irq[i] = platform_get_irq(pdev, i);
-   if (iommu->irq[i] < 0) {
-   dev_err(dev, "Failed to get IRQ, %d\n", iommu->irq[i]);
+   num_irq = of_irq_count(dev->of_node);


To follow up on the other reply, I'm not sure you really need to count 
the IRQs beforehand at all - you're going to be looping through 
platform_get_irq() and handling errors anyway, so you may as well just 
start at 0 and keep going until -ENOENT (or use platform_get_resource() 
to double-check whether an index should be valid, as we do in arm_smmu).


Otherwise, it looks like everything that the IRQ handler needs in the 
iommu struct (dev, num_mmu and bases) is already initialised by this 
point, so we should be OK with respect to races.


Robin.


+   for (i = 0; i < num_irq; i++) {
+   irq = platform_get_irq(pdev, i);
+   if (irq < 0) {
+   dev_err(dev, "Failed to get IRQ, %d\n", irq);
return -ENXIO;
}
+   err = devm_request_irq(iommu->dev, irq, rk_iommu_irq,
+  IRQF_SHARED, dev_name(dev), iommu);
+   if (err)
+   return err;
}
  
  	iommu->reset_disabled = device_property_read_bool(dev,

Re: [PATCH v2 01/13] iommu/rockchip: Request irqs in rk_iommu_probe()

2018-01-17 Thread Robin Murphy 

On 16/01/18 13:25, Jeffy Chen wrote:

Suggested-by: Robin Murphy 
Signed-off-by: Jeffy Chen 
---

Changes in v2: None

  drivers/iommu/rockchip-iommu.c | 38 +++---
  1 file changed, 11 insertions(+), 27 deletions(-)

diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
index 9d991c2d8767..4a12d4746095 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -18,6 +18,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  #include 
  #include 
@@ -91,7 +92,6 @@ struct rk_iommu {
void __iomem **bases;
int num_mmu;
int *irq;


Nit: irq seems to be redundant now as well.


-   int num_irq;
bool reset_disabled;
struct iommu_device iommu;
struct list_head node; /* entry in rk_iommu_domain.iommus */
@@ -830,13 +830,6 @@ static int rk_iommu_attach_device(struct iommu_domain 
*domain,
  
  	iommu->domain = domain;
  
-	for (i = 0; i < iommu->num_irq; i++) {

-   ret = devm_request_irq(iommu->dev, iommu->irq[i], rk_iommu_irq,
-  IRQF_SHARED, dev_name(dev), iommu);
-   if (ret)
-   return ret;
-   }
-
for (i = 0; i < iommu->num_mmu; i++) {
rk_iommu_write(iommu->bases[i], RK_MMU_DTE_ADDR,
   rk_domain->dt_dma);
@@ -885,9 +878,6 @@ static void rk_iommu_detach_device(struct iommu_domain 
*domain,
}
rk_iommu_disable_stall(iommu);
  
-	for (i = 0; i < iommu->num_irq; i++)

-   devm_free_irq(iommu->dev, iommu->irq[i], iommu);
-
iommu->domain = NULL;
  
  	dev_dbg(dev, "Detached from iommu domain\n");

@@ -1138,7 +1128,7 @@ static int rk_iommu_probe(struct platform_device *pdev)
struct rk_iommu *iommu;
struct resource *res;
int num_res = pdev->num_resources;
-   int err, i;
+   int err, i, irq, num_irq;
  
  	iommu = devm_kzalloc(dev, sizeof(*iommu), GFP_KERNEL);

if (!iommu)
@@ -1165,23 +1155,17 @@ static int rk_iommu_probe(struct platform_device *pdev)
if (iommu->num_mmu == 0)
return PTR_ERR(iommu->bases[0]);
  
-	iommu->num_irq = platform_irq_count(pdev);

-   if (iommu->num_irq < 0)
-   return iommu->num_irq;
-   if (iommu->num_irq == 0)
-   return -ENXIO;
-
-   iommu->irq = devm_kcalloc(dev, iommu->num_irq, sizeof(*iommu->irq),
- GFP_KERNEL);
-   if (!iommu->irq)
-   return -ENOMEM;
-
-   for (i = 0; i < iommu->num_irq; i++) {
-   iommu->irq[i] = platform_get_irq(pdev, i);
-   if (iommu->irq[i] < 0) {
-   dev_err(dev, "Failed to get IRQ, %d\n", iommu->irq[i]);
+   num_irq = of_irq_count(dev->of_node);


To follow up on the other reply, I'm not sure you really need to count 
the IRQs beforehand at all - you're going to be looping through 
platform_get_irq() and handling errors anyway, so you may as well just 
start at 0 and keep going until -ENOENT (or use platform_get_resource() 
to double-check whether an index should be valid, as we do in arm_smmu).


Otherwise, it looks like everything that the IRQ handler needs in the 
iommu struct (dev, num_mmu and bases) is already initialised by this 
point, so we should be OK with respect to races.


Robin.


+   for (i = 0; i < num_irq; i++) {
+   irq = platform_get_irq(pdev, i);
+   if (irq < 0) {
+   dev_err(dev, "Failed to get IRQ, %d\n", irq);
return -ENXIO;
}
+   err = devm_request_irq(iommu->dev, irq, rk_iommu_irq,
+  IRQF_SHARED, dev_name(dev), iommu);
+   if (err)
+   return err;
}
  
  	iommu->reset_disabled = device_property_read_bool(dev,

Re: [PATCH v2 01/13] iommu/rockchip: Request irqs in rk_iommu_probe()

2018-01-16 Thread JeffyChen 

Hi Tomasz,

On 01/17/2018 03:16 PM, Tomasz Figa wrote:

>>
>>This lacks consistency. of_irq_count() is used for counting, but
>>platform_get_irq() is used for getting. Either platform_ or of_ API
>>should be used for both and I'd lean for platform_, since it's more
>>general.

>
>hmmm, right, i was thinking of removing num_irq, and do something like:
>while (nr++) {
>   err = platform_get_irq(dev, nr);
>   if (err == -EPROBE_DEFER)
>  break;
>   if (err < 0)
>  return err;
>   
>}
>
>but forgot to do that..

Was there anything wrong with platform_irq_count() used by existing code?


just thought the platform_irq_count might ignore some errors(except for 
EPROBE_DEFER):


int platform_irq_count(struct platform_device *dev)
{
int ret, nr = 0;

while ((ret = platform_get_irq(dev, nr)) >= 0)
nr++;

if (ret == -EPROBE_DEFER)
return ret;

return nr;
}

but guess that would not matter..




>

>>

>>>+   if (irq < 0) {
>>>+   dev_err(dev, "Failed to get IRQ, %d\n", irq);
>>>  return -ENXIO;
>>>  }
>>>+   err = devm_request_irq(iommu->dev, irq, rk_iommu_irq,
>>>+  IRQF_SHARED, dev_name(dev),
>>>iommu);
>>>+   if (err)
>>>+   return err;
>>>  }

>>
>>
>>Looks like there is some more initialization below. Is the driver okay
>>with the IRQ being potentially fired right here? (Shared IRQ handlers
>>might be run at request_irq() time for testing.)
>>

>right, forget about that. maybe we can check iommu->domain not NULL in
>rk_iommu_irq()

Maybe we could just move IRQ requesting to the end of probe?


ok, that should work too.

and maybe we should also check power status in the irq handler? if it 
get fired after powered off...



Best regards,
Tomasz

Re: [PATCH v2 01/13] iommu/rockchip: Request irqs in rk_iommu_probe()

2018-01-16 Thread JeffyChen 

Hi Tomasz,

On 01/17/2018 03:16 PM, Tomasz Figa wrote:

>>
>>This lacks consistency. of_irq_count() is used for counting, but
>>platform_get_irq() is used for getting. Either platform_ or of_ API
>>should be used for both and I'd lean for platform_, since it's more
>>general.

>
>hmmm, right, i was thinking of removing num_irq, and do something like:
>while (nr++) {
>   err = platform_get_irq(dev, nr);
>   if (err == -EPROBE_DEFER)
>  break;
>   if (err < 0)
>  return err;
>   
>}
>
>but forgot to do that..

Was there anything wrong with platform_irq_count() used by existing code?


just thought the platform_irq_count might ignore some errors(except for 
EPROBE_DEFER):


int platform_irq_count(struct platform_device *dev)
{
int ret, nr = 0;

while ((ret = platform_get_irq(dev, nr)) >= 0)
nr++;

if (ret == -EPROBE_DEFER)
return ret;

return nr;
}

but guess that would not matter..




>

>>

>>>+   if (irq < 0) {
>>>+   dev_err(dev, "Failed to get IRQ, %d\n", irq);
>>>  return -ENXIO;
>>>  }
>>>+   err = devm_request_irq(iommu->dev, irq, rk_iommu_irq,
>>>+  IRQF_SHARED, dev_name(dev),
>>>iommu);
>>>+   if (err)
>>>+   return err;
>>>  }

>>
>>
>>Looks like there is some more initialization below. Is the driver okay
>>with the IRQ being potentially fired right here? (Shared IRQ handlers
>>might be run at request_irq() time for testing.)
>>

>right, forget about that. maybe we can check iommu->domain not NULL in
>rk_iommu_irq()

Maybe we could just move IRQ requesting to the end of probe?


ok, that should work too.

and maybe we should also check power status in the irq handler? if it 
get fired after powered off...



Best regards,
Tomasz

Re: [PATCH v2 01/13] iommu/rockchip: Request irqs in rk_iommu_probe()

2018-01-16 Thread Tomasz Figa 
On Wed, Jan 17, 2018 at 4:08 PM, JeffyChen  wrote:
> Hi Tomasz,
>
> Thanks for your reply.
>
> On 01/17/2018 12:21 PM, Tomasz Figa wrote:
>>
>> Hi Jeffy,
>>
>> Thanks for the patch. Please see my comments inline.
>>
>> On Tue, Jan 16, 2018 at 10:25 PM, Jeffy Chen 
>> wrote:
>>
>> Please add patch description.
>
>
> ok, will do.
>>
>>
>>> Suggested-by: Robin Murphy 
>>> Signed-off-by: Jeffy Chen 
>>> ---
>>
>> [snip]
>>>
>>> -   for (i = 0; i < iommu->num_irq; i++) {
>>> -   iommu->irq[i] = platform_get_irq(pdev, i);
>>> -   if (iommu->irq[i] < 0) {
>>> -   dev_err(dev, "Failed to get IRQ, %d\n",
>>> iommu->irq[i]);
>>> +   num_irq = of_irq_count(dev->of_node);
>>> +   for (i = 0; i < num_irq; i++) {
>>> +   irq = platform_get_irq(pdev, i);
>>
>>
>> This lacks consistency. of_irq_count() is used for counting, but
>> platform_get_irq() is used for getting. Either platform_ or of_ API
>> should be used for both and I'd lean for platform_, since it's more
>> general.
>
> hmmm, right, i was thinking of removing num_irq, and do something like:
> while (nr++) {
>   err = platform_get_irq(dev, nr);
>   if (err == -EPROBE_DEFER)
>  break;
>   if (err < 0)
>  return err;
>   
> }
>
> but forgot to do that..

Was there anything wrong with platform_irq_count() used by existing code?

>
>>
>>> +   if (irq < 0) {
>>> +   dev_err(dev, "Failed to get IRQ, %d\n", irq);
>>>  return -ENXIO;
>>>  }
>>> +   err = devm_request_irq(iommu->dev, irq, rk_iommu_irq,
>>> +  IRQF_SHARED, dev_name(dev),
>>> iommu);
>>> +   if (err)
>>> +   return err;
>>>  }
>>
>>
>> Looks like there is some more initialization below. Is the driver okay
>> with the IRQ being potentially fired right here? (Shared IRQ handlers
>> might be run at request_irq() time for testing.)
>>
> right, forget about that. maybe we can check iommu->domain not NULL in
> rk_iommu_irq()

Maybe we could just move IRQ requesting to the end of probe?

Best regards,
Tomasz

Re: [PATCH v2 01/13] iommu/rockchip: Request irqs in rk_iommu_probe()

2018-01-16 Thread Tomasz Figa 
On Wed, Jan 17, 2018 at 4:08 PM, JeffyChen  wrote:
> Hi Tomasz,
>
> Thanks for your reply.
>
> On 01/17/2018 12:21 PM, Tomasz Figa wrote:
>>
>> Hi Jeffy,
>>
>> Thanks for the patch. Please see my comments inline.
>>
>> On Tue, Jan 16, 2018 at 10:25 PM, Jeffy Chen 
>> wrote:
>>
>> Please add patch description.
>
>
> ok, will do.
>>
>>
>>> Suggested-by: Robin Murphy 
>>> Signed-off-by: Jeffy Chen 
>>> ---
>>
>> [snip]
>>>
>>> -   for (i = 0; i < iommu->num_irq; i++) {
>>> -   iommu->irq[i] = platform_get_irq(pdev, i);
>>> -   if (iommu->irq[i] < 0) {
>>> -   dev_err(dev, "Failed to get IRQ, %d\n",
>>> iommu->irq[i]);
>>> +   num_irq = of_irq_count(dev->of_node);
>>> +   for (i = 0; i < num_irq; i++) {
>>> +   irq = platform_get_irq(pdev, i);
>>
>>
>> This lacks consistency. of_irq_count() is used for counting, but
>> platform_get_irq() is used for getting. Either platform_ or of_ API
>> should be used for both and I'd lean for platform_, since it's more
>> general.
>
> hmmm, right, i was thinking of removing num_irq, and do something like:
> while (nr++) {
>   err = platform_get_irq(dev, nr);
>   if (err == -EPROBE_DEFER)
>  break;
>   if (err < 0)
>  return err;
>   
> }
>
> but forgot to do that..

Was there anything wrong with platform_irq_count() used by existing code?

>
>>
>>> +   if (irq < 0) {
>>> +   dev_err(dev, "Failed to get IRQ, %d\n", irq);
>>>  return -ENXIO;
>>>  }
>>> +   err = devm_request_irq(iommu->dev, irq, rk_iommu_irq,
>>> +  IRQF_SHARED, dev_name(dev),
>>> iommu);
>>> +   if (err)
>>> +   return err;
>>>  }
>>
>>
>> Looks like there is some more initialization below. Is the driver okay
>> with the IRQ being potentially fired right here? (Shared IRQ handlers
>> might be run at request_irq() time for testing.)
>>
> right, forget about that. maybe we can check iommu->domain not NULL in
> rk_iommu_irq()

Maybe we could just move IRQ requesting to the end of probe?

Best regards,
Tomasz

Re: [PATCH v2 01/13] iommu/rockchip: Request irqs in rk_iommu_probe()

2018-01-16 Thread Tomasz Figa 
Hi Jeffy,

Thanks for the patch. Please see my comments inline.

On Tue, Jan 16, 2018 at 10:25 PM, Jeffy Chen  wrote:

Please add patch description.

> Suggested-by: Robin Murphy 
> Signed-off-by: Jeffy Chen 
> ---
[snip]
> -   for (i = 0; i < iommu->num_irq; i++) {
> -   iommu->irq[i] = platform_get_irq(pdev, i);
> -   if (iommu->irq[i] < 0) {
> -   dev_err(dev, "Failed to get IRQ, %d\n", 
> iommu->irq[i]);
> +   num_irq = of_irq_count(dev->of_node);
> +   for (i = 0; i < num_irq; i++) {
> +   irq = platform_get_irq(pdev, i);

This lacks consistency. of_irq_count() is used for counting, but
platform_get_irq() is used for getting. Either platform_ or of_ API
should be used for both and I'd lean for platform_, since it's more
general.

> +   if (irq < 0) {
> +   dev_err(dev, "Failed to get IRQ, %d\n", irq);
> return -ENXIO;
> }
> +   err = devm_request_irq(iommu->dev, irq, rk_iommu_irq,
> +  IRQF_SHARED, dev_name(dev), iommu);
> +   if (err)
> +   return err;
> }

Looks like there is some more initialization below. Is the driver okay
with the IRQ being potentially fired right here? (Shared IRQ handlers
might be run at request_irq() time for testing.)

>
> iommu->reset_disabled = device_property_read_bool(dev,

^^

Best regards,
Tomasz

Re: [PATCH v2 01/13] iommu/rockchip: Request irqs in rk_iommu_probe()

2018-01-16 Thread Tomasz Figa 
Hi Jeffy,

Thanks for the patch. Please see my comments inline.

On Tue, Jan 16, 2018 at 10:25 PM, Jeffy Chen  wrote:

Please add patch description.

> Suggested-by: Robin Murphy 
> Signed-off-by: Jeffy Chen 
> ---
[snip]
> -   for (i = 0; i < iommu->num_irq; i++) {
> -   iommu->irq[i] = platform_get_irq(pdev, i);
> -   if (iommu->irq[i] < 0) {
> -   dev_err(dev, "Failed to get IRQ, %d\n", 
> iommu->irq[i]);
> +   num_irq = of_irq_count(dev->of_node);
> +   for (i = 0; i < num_irq; i++) {
> +   irq = platform_get_irq(pdev, i);

This lacks consistency. of_irq_count() is used for counting, but
platform_get_irq() is used for getting. Either platform_ or of_ API
should be used for both and I'd lean for platform_, since it's more
general.

> +   if (irq < 0) {
> +   dev_err(dev, "Failed to get IRQ, %d\n", irq);
> return -ENXIO;
> }
> +   err = devm_request_irq(iommu->dev, irq, rk_iommu_irq,
> +  IRQF_SHARED, dev_name(dev), iommu);
> +   if (err)
> +   return err;
> }

Looks like there is some more initialization below. Is the driver okay
with the IRQ being potentially fired right here? (Shared IRQ handlers
might be run at request_irq() time for testing.)

>
> iommu->reset_disabled = device_property_read_bool(dev,

^^

Best regards,
Tomasz

[PATCH v2 01/13] iommu/rockchip: Request irqs in rk_iommu_probe()

2018-01-16 Thread Jeffy Chen 
Suggested-by: Robin Murphy 
Signed-off-by: Jeffy Chen 
---

Changes in v2: None

 drivers/iommu/rockchip-iommu.c | 38 +++---
 1 file changed, 11 insertions(+), 27 deletions(-)

diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
index 9d991c2d8767..4a12d4746095 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -18,6 +18,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -91,7 +92,6 @@ struct rk_iommu {
void __iomem **bases;
int num_mmu;
int *irq;
-   int num_irq;
bool reset_disabled;
struct iommu_device iommu;
struct list_head node; /* entry in rk_iommu_domain.iommus */
@@ -830,13 +830,6 @@ static int rk_iommu_attach_device(struct iommu_domain 
*domain,
 
iommu->domain = domain;
 
-   for (i = 0; i < iommu->num_irq; i++) {
-   ret = devm_request_irq(iommu->dev, iommu->irq[i], rk_iommu_irq,
-  IRQF_SHARED, dev_name(dev), iommu);
-   if (ret)
-   return ret;
-   }
-
for (i = 0; i < iommu->num_mmu; i++) {
rk_iommu_write(iommu->bases[i], RK_MMU_DTE_ADDR,
   rk_domain->dt_dma);
@@ -885,9 +878,6 @@ static void rk_iommu_detach_device(struct iommu_domain 
*domain,
}
rk_iommu_disable_stall(iommu);
 
-   for (i = 0; i < iommu->num_irq; i++)
-   devm_free_irq(iommu->dev, iommu->irq[i], iommu);
-
iommu->domain = NULL;
 
dev_dbg(dev, "Detached from iommu domain\n");
@@ -1138,7 +1128,7 @@ static int rk_iommu_probe(struct platform_device *pdev)
struct rk_iommu *iommu;
struct resource *res;
int num_res = pdev->num_resources;
-   int err, i;
+   int err, i, irq, num_irq;
 
iommu = devm_kzalloc(dev, sizeof(*iommu), GFP_KERNEL);
if (!iommu)
@@ -1165,23 +1155,17 @@ static int rk_iommu_probe(struct platform_device *pdev)
if (iommu->num_mmu == 0)
return PTR_ERR(iommu->bases[0]);
 
-   iommu->num_irq = platform_irq_count(pdev);
-   if (iommu->num_irq < 0)
-   return iommu->num_irq;
-   if (iommu->num_irq == 0)
-   return -ENXIO;
-
-   iommu->irq = devm_kcalloc(dev, iommu->num_irq, sizeof(*iommu->irq),
- GFP_KERNEL);
-   if (!iommu->irq)
-   return -ENOMEM;
-
-   for (i = 0; i < iommu->num_irq; i++) {
-   iommu->irq[i] = platform_get_irq(pdev, i);
-   if (iommu->irq[i] < 0) {
-   dev_err(dev, "Failed to get IRQ, %d\n", iommu->irq[i]);
+   num_irq = of_irq_count(dev->of_node);
+   for (i = 0; i < num_irq; i++) {
+   irq = platform_get_irq(pdev, i);
+   if (irq < 0) {
+   dev_err(dev, "Failed to get IRQ, %d\n", irq);
return -ENXIO;
}
+   err = devm_request_irq(iommu->dev, irq, rk_iommu_irq,
+  IRQF_SHARED, dev_name(dev), iommu);
+   if (err)
+   return err;
}
 
iommu->reset_disabled = device_property_read_bool(dev,
-- 
2.11.0

[PATCH v2 01/13] iommu/rockchip: Request irqs in rk_iommu_probe()

2018-01-16 Thread Jeffy Chen 
Suggested-by: Robin Murphy 
Signed-off-by: Jeffy Chen 
---

Changes in v2: None

 drivers/iommu/rockchip-iommu.c | 38 +++---
 1 file changed, 11 insertions(+), 27 deletions(-)

diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
index 9d991c2d8767..4a12d4746095 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -18,6 +18,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -91,7 +92,6 @@ struct rk_iommu {
void __iomem **bases;
int num_mmu;
int *irq;
-   int num_irq;
bool reset_disabled;
struct iommu_device iommu;
struct list_head node; /* entry in rk_iommu_domain.iommus */
@@ -830,13 +830,6 @@ static int rk_iommu_attach_device(struct iommu_domain 
*domain,
 
iommu->domain = domain;
 
-   for (i = 0; i < iommu->num_irq; i++) {
-   ret = devm_request_irq(iommu->dev, iommu->irq[i], rk_iommu_irq,
-  IRQF_SHARED, dev_name(dev), iommu);
-   if (ret)
-   return ret;
-   }
-
for (i = 0; i < iommu->num_mmu; i++) {
rk_iommu_write(iommu->bases[i], RK_MMU_DTE_ADDR,
   rk_domain->dt_dma);
@@ -885,9 +878,6 @@ static void rk_iommu_detach_device(struct iommu_domain 
*domain,
}
rk_iommu_disable_stall(iommu);
 
-   for (i = 0; i < iommu->num_irq; i++)
-   devm_free_irq(iommu->dev, iommu->irq[i], iommu);
-
iommu->domain = NULL;
 
dev_dbg(dev, "Detached from iommu domain\n");
@@ -1138,7 +1128,7 @@ static int rk_iommu_probe(struct platform_device *pdev)
struct rk_iommu *iommu;
struct resource *res;
int num_res = pdev->num_resources;
-   int err, i;
+   int err, i, irq, num_irq;
 
iommu = devm_kzalloc(dev, sizeof(*iommu), GFP_KERNEL);
if (!iommu)
@@ -1165,23 +1155,17 @@ static int rk_iommu_probe(struct platform_device *pdev)
if (iommu->num_mmu == 0)
return PTR_ERR(iommu->bases[0]);
 
-   iommu->num_irq = platform_irq_count(pdev);
-   if (iommu->num_irq < 0)
-   return iommu->num_irq;
-   if (iommu->num_irq == 0)
-   return -ENXIO;
-
-   iommu->irq = devm_kcalloc(dev, iommu->num_irq, sizeof(*iommu->irq),
- GFP_KERNEL);
-   if (!iommu->irq)
-   return -ENOMEM;
-
-   for (i = 0; i < iommu->num_irq; i++) {
-   iommu->irq[i] = platform_get_irq(pdev, i);
-   if (iommu->irq[i] < 0) {
-   dev_err(dev, "Failed to get IRQ, %d\n", iommu->irq[i]);
+   num_irq = of_irq_count(dev->of_node);
+   for (i = 0; i < num_irq; i++) {
+   irq = platform_get_irq(pdev, i);
+   if (irq < 0) {
+   dev_err(dev, "Failed to get IRQ, %d\n", irq);
return -ENXIO;
}
+   err = devm_request_irq(iommu->dev, irq, rk_iommu_irq,
+  IRQF_SHARED, dev_name(dev), iommu);
+   if (err)
+   return err;
}
 
iommu->reset_disabled = device_property_read_bool(dev,
-- 
2.11.0