Re: [PATCH 2/2] staging: fsl-mc: do not print error in case of defer probe error

Re: [PATCH 2/2] staging: fsl-mc: do not print error in case of defer probe error

[Greg KH]

On Wed, Dec 06, 2017 at 09:48:07PM +0530, Nipun Gupta wrote:
> Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
> ---

I can't take patches without any changelog text :(

Please fix and resend the series.

greg k-h

Re: [PATCH 1/2] staging: fsl-mc: Allocate IRQ's before scanning DPRC objects

[Laurentiu Tudor]

Hi Nipun,

Can you polish a bit this commit message? It doesn't seem to explain why 
this is needed.

On 12/06/2017 06:18 PM, Nipun Gupta wrote:
> When DPRC probing is deferred (such as where IOMMU is not probed
> before the fsl-mc bus), all the devices in the DPRC containers gets
> initialized one after another.

Are you referring to dprc probing being deferred (do we ever do that?) 
or devices inside the dprc deferring the probe?

> As IRQ's were allocated only once the
> DPRC scanning is completed, the devices like DPIO which uses these
> IRQ's at initalization fails. This patch allocates the IRQ resources

s/initalization/initialization

> before scanning all the objects in the DPRC container.
>
> Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
> ---
>   drivers/staging/fsl-mc/bus/dprc-driver.c | 49 ++++++++++++++++++--------------
>   1 file changed, 27 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/staging/fsl-mc/bus/dprc-driver.c b/drivers/staging/fsl-mc/bus/dprc-driver.c
> index 06df528..7265431 100644
> --- a/drivers/staging/fsl-mc/bus/dprc-driver.c
> +++ b/drivers/staging/fsl-mc/bus/dprc-driver.c
> @@ -206,7 +206,8 @@ static void dprc_add_new_devices(struct fsl_mc_device *mc_bus_dev,
>    * dprc_scan_objects - Discover objects in a DPRC
>    *
>    * @mc_bus_dev: pointer to the fsl-mc device that represents a DPRC object
> - * @total_irq_count: total number of IRQs needed by objects in the DPRC.
> + * @total_irq_count: If argument is provided the function populates the
> + * total number of IRQs created by objects in the DPRC.

As a side node, after a cursory look i noticed that this total_irq_count 
parameter is used only for some sanity checks. I'm thinking of dropping 
it in a follow-up cleanup patch.

---
Best Regards, Laurentiu

RE: [PATCH 1/2] staging: fsl-mc: Allocate IRQ's before scanning DPRC objects

[Bharat Bhushan]

Hi Laurentiu,

> -----Original Message-----
> From: Laurentiu Tudor
> Sent: Wednesday, December 06, 2017 7:00 PM
> To: Nipun Gupta <nipun.gupta@nxp.com>; stuyoder@gmail.com; Bharat
> Bhushan <bharat.bhushan@nxp.com>; gregkh@linuxfoundation.org;
> cakturk@gmail.com; bretth256@gmail.com; arnd@arndb.de
> Cc: linux-kernel@vger.kernel.org; devel@driverdev.osuosl.org
> Subject: Re: [PATCH 1/2] staging: fsl-mc: Allocate IRQ's before scanning DPRC
> objects
> 
> Hi Nipun,
> 
> Can you polish a bit this commit message? It doesn't seem to explain why this
> is needed.
> 
> On 12/06/2017 06:18 PM, Nipun Gupta wrote:
> > When DPRC probing is deferred (such as where IOMMU is not probed
> > before the fsl-mc bus), all the devices in the DPRC containers gets
> > initialized one after another.
> 
> Are you referring to dprc probing being deferred (do we ever do that?) or
> devices inside the dprc deferring the probe?
> 
> > As IRQ's were allocated only once the
> > DPRC scanning is completed, the devices like DPIO which uses these
> > IRQ's at initalization fails. This patch allocates the IRQ resources
> 
> s/initalization/initialization
> 
> > before scanning all the objects in the DPRC container.
> >
> > Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
> > ---
> >   drivers/staging/fsl-mc/bus/dprc-driver.c | 49 ++++++++++++++++++------
> --------
> >   1 file changed, 27 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/staging/fsl-mc/bus/dprc-driver.c
> > b/drivers/staging/fsl-mc/bus/dprc-driver.c
> > index 06df528..7265431 100644
> > --- a/drivers/staging/fsl-mc/bus/dprc-driver.c
> > +++ b/drivers/staging/fsl-mc/bus/dprc-driver.c
> > @@ -206,7 +206,8 @@ static void dprc_add_new_devices(struct
> fsl_mc_device *mc_bus_dev,
> >    * dprc_scan_objects - Discover objects in a DPRC
> >    *
> >    * @mc_bus_dev: pointer to the fsl-mc device that represents a DPRC
> > object
> > - * @total_irq_count: total number of IRQs needed by objects in the DPRC.
> > + * @total_irq_count: If argument is provided the function populates
> > + the
> > + * total number of IRQs created by objects in the DPRC.
> 
> As a side node, after a cursory look i noticed that this total_irq_count
> parameter is used only for some sanity checks. I'm thinking of dropping it in a
> follow-up cleanup patch.

How you will ensure that number of IRQ needed are not sufficient for devices in the container?
Do you think we need to either not enable additional devices or add irqs to irq-pool ?

Thanks
-Bharat

> 
> ---
> Best Regards, Laurentiu

Re: [PATCH 1/2] staging: fsl-mc: Allocate IRQ's before scanning DPRC objects

[Laurentiu Tudor]

Hi Bharat,

On 12/06/2017 04:03 PM, Bharat Bhushan wrote:
> Hi Laurentiu,
>
>> -----Original Message-----
>> From: Laurentiu Tudor
>> Sent: Wednesday, December 06, 2017 7:00 PM
>> To: Nipun Gupta <nipun.gupta@nxp.com>; stuyoder@gmail.com; Bharat
>> Bhushan <bharat.bhushan@nxp.com>; gregkh@linuxfoundation.org;
>> cakturk@gmail.com; bretth256@gmail.com; arnd@arndb.de
>> Cc: linux-kernel@vger.kernel.org; devel@driverdev.osuosl.org
>> Subject: Re: [PATCH 1/2] staging: fsl-mc: Allocate IRQ's before scanning DPRC
>> objects
>>
>> Hi Nipun,
>>
>> Can you polish a bit this commit message? It doesn't seem to explain why this
>> is needed.
>>
>> On 12/06/2017 06:18 PM, Nipun Gupta wrote:
>>> When DPRC probing is deferred (such as where IOMMU is not probed
>>> before the fsl-mc bus), all the devices in the DPRC containers gets
>>> initialized one after another.
>>
>> Are you referring to dprc probing being deferred (do we ever do that?) or
>> devices inside the dprc deferring the probe?
>>
>>> As IRQ's were allocated only once the
>>> DPRC scanning is completed, the devices like DPIO which uses these
>>> IRQ's at initalization fails. This patch allocates the IRQ resources
>>
>> s/initalization/initialization
>>
>>> before scanning all the objects in the DPRC container.
>>>
>>> Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
>>> ---
>>>    drivers/staging/fsl-mc/bus/dprc-driver.c | 49 ++++++++++++++++++------
>> --------
>>>    1 file changed, 27 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/drivers/staging/fsl-mc/bus/dprc-driver.c
>>> b/drivers/staging/fsl-mc/bus/dprc-driver.c
>>> index 06df528..7265431 100644
>>> --- a/drivers/staging/fsl-mc/bus/dprc-driver.c
>>> +++ b/drivers/staging/fsl-mc/bus/dprc-driver.c
>>> @@ -206,7 +206,8 @@ static void dprc_add_new_devices(struct
>> fsl_mc_device *mc_bus_dev,
>>>     * dprc_scan_objects - Discover objects in a DPRC
>>>     *
>>>     * @mc_bus_dev: pointer to the fsl-mc device that represents a DPRC
>>> object
>>> - * @total_irq_count: total number of IRQs needed by objects in the DPRC.
>>> + * @total_irq_count: If argument is provided the function populates
>>> + the
>>> + * total number of IRQs created by objects in the DPRC.
>>
>> As a side node, after a cursory look i noticed that this total_irq_count
>> parameter is used only for some sanity checks. I'm thinking of dropping it in a
>> follow-up cleanup patch.
>
> How you will ensure that number of IRQ needed are not sufficient for devices in the container?
> Do you think we need to either not enable additional devices or add irqs to irq-pool ?

In the current implementation we allocate a pool of 
FSL_MC_IRQ_POOL_MAX_TOTAL_IRQS (= 256) no mater what total_irq_count is.
I think this is enough for our current scenarios, but if in the future 
we ever hit this limit we can think of a mechanism along the lines of 
your example. Adding another chunk of irqs to the pool seems to me like 
a good idea in the future.

---
Best Regards, Laurentiu

[PATCH 1/2] staging: fsl-mc: Allocate IRQ's before scanning DPRC objects

[Nipun Gupta]

When DPRC probing is deferred (such as where IOMMU is not probed
before the fsl-mc bus), all the devices in the DPRC containers gets
initialized one after another. As IRQ's were allocated only once the
DPRC scanning is completed, the devices like DPIO which uses these
IRQ's at initalization fails. This patch allocates the IRQ resources
before scanning all the objects in the DPRC container.

Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
---
 drivers/staging/fsl-mc/bus/dprc-driver.c | 49 ++++++++++++++++++--------------
 1 file changed, 27 insertions(+), 22 deletions(-)

diff --git a/drivers/staging/fsl-mc/bus/dprc-driver.c b/drivers/staging/fsl-mc/bus/dprc-driver.c
index 06df528..7265431 100644
--- a/drivers/staging/fsl-mc/bus/dprc-driver.c
+++ b/drivers/staging/fsl-mc/bus/dprc-driver.c
@@ -206,7 +206,8 @@ static void dprc_add_new_devices(struct fsl_mc_device *mc_bus_dev,
  * dprc_scan_objects - Discover objects in a DPRC
  *
  * @mc_bus_dev: pointer to the fsl-mc device that represents a DPRC object
- * @total_irq_count: total number of IRQs needed by objects in the DPRC.
+ * @total_irq_count: If argument is provided the function populates the
+ * total number of IRQs created by objects in the DPRC.
  *
  * Detects objects added and removed from a DPRC and synchronizes the
  * state of the Linux bus driver, MC by adding and removing
@@ -228,6 +229,7 @@ static int dprc_scan_objects(struct fsl_mc_device *mc_bus_dev,
 	int error;
 	unsigned int irq_count = mc_bus_dev->obj_desc.irq_count;
 	struct fsl_mc_obj_desc *child_obj_desc_array = NULL;
+	struct fsl_mc_bus *mc_bus = to_fsl_mc_bus(mc_bus_dev);
 
 	error = dprc_get_obj_count(mc_bus_dev->mc_io,
 				   0,
@@ -297,7 +299,26 @@ static int dprc_scan_objects(struct fsl_mc_device *mc_bus_dev,
 		}
 	}
 
-	*total_irq_count = irq_count;
+	/*
+	 * Allocate IRQ's before scanning objects in DPRC as some devices like
+	 * DPIO needs them on being probed.
+	 */
+	if (dev_get_msi_domain(&mc_bus_dev->dev) && !mc_bus->irq_resources) {
+		if (irq_count > FSL_MC_IRQ_POOL_MAX_TOTAL_IRQS) {
+			dev_warn(&mc_bus_dev->dev,
+				 "IRQs needed (%u) exceed IRQs preallocated (%u)\n",
+				 irq_count, FSL_MC_IRQ_POOL_MAX_TOTAL_IRQS);
+		}
+
+		error = fsl_mc_populate_irq_pool(mc_bus,
+				FSL_MC_IRQ_POOL_MAX_TOTAL_IRQS);
+		if (error < 0)
+			return error;
+	}
+
+	if (total_irq_count)
+		*total_irq_count = irq_count;
+
 	dprc_remove_devices(mc_bus_dev, child_obj_desc_array,
 			    num_child_objects);
 
@@ -322,7 +343,6 @@ static int dprc_scan_objects(struct fsl_mc_device *mc_bus_dev,
 static int dprc_scan_container(struct fsl_mc_device *mc_bus_dev)
 {
 	int error;
-	unsigned int irq_count;
 	struct fsl_mc_bus *mc_bus = to_fsl_mc_bus(mc_bus_dev);
 
 	fsl_mc_init_all_resource_pools(mc_bus_dev);
@@ -331,29 +351,14 @@ static int dprc_scan_container(struct fsl_mc_device *mc_bus_dev)
 	 * Discover objects in the DPRC:
 	 */
 	mutex_lock(&mc_bus->scan_mutex);
-	error = dprc_scan_objects(mc_bus_dev, &irq_count);
+	error = dprc_scan_objects(mc_bus_dev, NULL);
 	mutex_unlock(&mc_bus->scan_mutex);
-	if (error < 0)
-		goto error;
-
-	if (dev_get_msi_domain(&mc_bus_dev->dev) && !mc_bus->irq_resources) {
-		if (irq_count > FSL_MC_IRQ_POOL_MAX_TOTAL_IRQS) {
-			dev_warn(&mc_bus_dev->dev,
-				 "IRQs needed (%u) exceed IRQs preallocated (%u)\n",
-				 irq_count, FSL_MC_IRQ_POOL_MAX_TOTAL_IRQS);
-		}
-
-		error = fsl_mc_populate_irq_pool(
-				mc_bus,
-				FSL_MC_IRQ_POOL_MAX_TOTAL_IRQS);
-		if (error < 0)
-			goto error;
+	if (error < 0) {
+		fsl_mc_cleanup_all_resource_pools(mc_bus_dev);
+		return error;
 	}
 
 	return 0;
-error:
-	fsl_mc_cleanup_all_resource_pools(mc_bus_dev);
-	return error;
 }
 
 /**
-- 
1.9.1

[PATCH 2/2] staging: fsl-mc: do not print error in case of defer probe error

[Nipun Gupta]

Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
---
 drivers/staging/fsl-mc/bus/fsl-mc-bus.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
index 409f2b9..10cee00 100644
--- a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
+++ b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
@@ -171,7 +171,8 @@ static int fsl_mc_driver_probe(struct device *dev)
 
 	error = mc_drv->probe(mc_dev);
 	if (error < 0) {
-		dev_err(dev, "%s failed: %d\n", __func__, error);
+		if (error != -EPROBE_DEFER)
+			dev_err(dev, "%s failed: %d\n", __func__, error);
 		return error;
 	}
 
-- 
1.9.1

RE: [PATCH 2/2] staging: fsl-mc: do not print error in case of defer probe error

[Nipun Gupta]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Wednesday, December 06, 2017 16:46
> To: Nipun Gupta <nipun.gupta@nxp.com>
> Cc: Laurentiu Tudor <laurentiu.tudor@nxp.com>; stuyoder@gmail.com; Bharat
> Bhushan <bharat.bhushan@nxp.com>; cakturk@gmail.com;
> bretth256@gmail.com; arnd@arndb.de; devel@driverdev.osuosl.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH 2/2] staging: fsl-mc: do not print error in case of defer
> probe error
> 
> On Wed, Dec 06, 2017 at 09:48:07PM +0530, Nipun Gupta wrote:
> > Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
> > ---
> 
> I can't take patches without any changelog text :(

I thought it is a small patch and you may take it without any changelog ;)
Ill resend updating this.

Thanks,
Nipun

> 
> Please fix and resend the series.
> 
> greg k-h

RE: [PATCH 1/2] staging: fsl-mc: Allocate IRQ's before scanning DPRC objects

[Nipun Gupta]

> -----Original Message-----
> From: Laurentiu Tudor
> Sent: Wednesday, December 06, 2017 19:00
> To: Nipun Gupta <nipun.gupta@nxp.com>; stuyoder@gmail.com; Bharat
> Bhushan <bharat.bhushan@nxp.com>; gregkh@linuxfoundation.org;
> cakturk@gmail.com; bretth256@gmail.com; arnd@arndb.de
> Cc: linux-kernel@vger.kernel.org; devel@driverdev.osuosl.org
> Subject: Re: [PATCH 1/2] staging: fsl-mc: Allocate IRQ's before scanning DPRC
> objects
> 
> Hi Nipun,
> 
> Can you polish a bit this commit message? It doesn't seem to explain why
> this is needed.

Sure. Ill update the message.

> 
> On 12/06/2017 06:18 PM, Nipun Gupta wrote:
> > When DPRC probing is deferred (such as where IOMMU is not probed
> > before the fsl-mc bus), all the devices in the DPRC containers gets
> > initialized one after another.
> 
> Are you referring to dprc probing being deferred (do we ever do that?)
> or devices inside the dprc deferring the probe?

Yes.. Currently following is the scenario when the devices are probed
(please correct me if I am wrong):

	FSL_MC Bus probe ---> dprc probe ---> dprc devices scan --->
		probe of devices in DPRC container ---> allocate IRQ's.

In case the devices being probed in the DPRC container need the IRQ's;
probing of that device will fail.
In the current scenario DPIO device while getting probed for the first time
gets deferred because the DPIO driver is not yet registered.
So there is no impact seen in the current code.

In case the DPRC probing gets deferred, does in case IOMMU is enabled
(though it is not enabled till now for fsl-mc bus but we plan to add the
support as soon as mc bus is out from staging); the DPIO gets probed
before IRQ's being allocated. This causes DPIO probe to fail.

So I think it makes sense that IRQ's are allocated before any devices
In the DPRC container are probed.

> 
> > As IRQ's were allocated only once the
> > DPRC scanning is completed, the devices like DPIO which uses these
> > IRQ's at initalization fails. This patch allocates the IRQ resources
> 
> s/initalization/initialization
> 
> > before scanning all the objects in the DPRC container.
> >
> > Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
> > ---
> >   drivers/staging/fsl-mc/bus/dprc-driver.c | 49 ++++++++++++++++++------------
> --
> >   1 file changed, 27 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/staging/fsl-mc/bus/dprc-driver.c b/drivers/staging/fsl-
> mc/bus/dprc-driver.c
> > index 06df528..7265431 100644
> > --- a/drivers/staging/fsl-mc/bus/dprc-driver.c
> > +++ b/drivers/staging/fsl-mc/bus/dprc-driver.c
> > @@ -206,7 +206,8 @@ static void dprc_add_new_devices(struct
> fsl_mc_device *mc_bus_dev,
> >    * dprc_scan_objects - Discover objects in a DPRC
> >    *
> >    * @mc_bus_dev: pointer to the fsl-mc device that represents a DPRC object
> > - * @total_irq_count: total number of IRQs needed by objects in the DPRC.
> > + * @total_irq_count: If argument is provided the function populates the
> > + * total number of IRQs created by objects in the DPRC.
> 
> As a side node, after a cursory look i noticed that this total_irq_count
> parameter is used only for some sanity checks. I'm thinking of dropping
> it in a follow-up cleanup patch.

Do you plan to send it very recently.
In that case I can rebase my patch on top of it.

Regards,
Nipun

> 
> ---
> Best Regards, Laurentiu

Re: [PATCH 1/2] staging: fsl-mc: Allocate IRQ's before scanning DPRC objects

[Laurentiu Tudor]

On 12/07/2017 03:18 PM, Nipun Gupta wrote:
>
>
>> -----Original Message-----
>> From: Laurentiu Tudor
>> Sent: Wednesday, December 06, 2017 19:00
>> To: Nipun Gupta <nipun.gupta@nxp.com>; stuyoder@gmail.com; Bharat
>> Bhushan <bharat.bhushan@nxp.com>; gregkh@linuxfoundation.org;
>> cakturk@gmail.com; bretth256@gmail.com; arnd@arndb.de
>> Cc: linux-kernel@vger.kernel.org; devel@driverdev.osuosl.org
>> Subject: Re: [PATCH 1/2] staging: fsl-mc: Allocate IRQ's before scanning DPRC
>> objects
>>
>> Hi Nipun,
>>
>> Can you polish a bit this commit message? It doesn't seem to explain why
>> this is needed.
>
> Sure. Ill update the message.
>
>>
>> On 12/06/2017 06:18 PM, Nipun Gupta wrote:
>>> When DPRC probing is deferred (such as where IOMMU is not probed
>>> before the fsl-mc bus), all the devices in the DPRC containers gets
>>> initialized one after another.
>>
>> Are you referring to dprc probing being deferred (do we ever do that?)
>> or devices inside the dprc deferring the probe?
>
> Yes.. Currently following is the scenario when the devices are probed
> (please correct me if I am wrong):
>
> 	FSL_MC Bus probe ---> dprc probe ---> dprc devices scan --->
> 		probe of devices in DPRC container ---> allocate IRQ's.
>
> In case the devices being probed in the DPRC container need the IRQ's;
> probing of that device will fail.
> In the current scenario DPIO device while getting probed for the first time
> gets deferred because the DPIO driver is not yet registered.
> So there is no impact seen in the current code.
>
> In case the DPRC probing gets deferred, does in case IOMMU is enabled
> (though it is not enabled till now for fsl-mc bus but we plan to add the
> support as soon as mc bus is out from staging); the DPIO gets probed
> before IRQ's being allocated. This causes DPIO probe to fail.
>
> So I think it makes sense that IRQ's are allocated before any devices
> In the DPRC container are probed.

Thanks for the details. It would be great if you could update the commit 
message with these execution flow details.

>>
>>> As IRQ's were allocated only once the
>>> DPRC scanning is completed, the devices like DPIO which uses these
>>> IRQ's at initalization fails. This patch allocates the IRQ resources
>>
>> s/initalization/initialization
>>
>>> before scanning all the objects in the DPRC container.
>>>
>>> Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
>>> ---
>>>    drivers/staging/fsl-mc/bus/dprc-driver.c | 49 ++++++++++++++++++------------
>> --
>>>    1 file changed, 27 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/drivers/staging/fsl-mc/bus/dprc-driver.c b/drivers/staging/fsl-
>> mc/bus/dprc-driver.c
>>> index 06df528..7265431 100644
>>> --- a/drivers/staging/fsl-mc/bus/dprc-driver.c
>>> +++ b/drivers/staging/fsl-mc/bus/dprc-driver.c
>>> @@ -206,7 +206,8 @@ static void dprc_add_new_devices(struct
>> fsl_mc_device *mc_bus_dev,
>>>     * dprc_scan_objects - Discover objects in a DPRC
>>>     *
>>>     * @mc_bus_dev: pointer to the fsl-mc device that represents a DPRC object
>>> - * @total_irq_count: total number of IRQs needed by objects in the DPRC.
>>> + * @total_irq_count: If argument is provided the function populates the
>>> + * total number of IRQs created by objects in the DPRC.
>>
>> As a side node, after a cursory look i noticed that this total_irq_count
>> parameter is used only for some sanity checks. I'm thinking of dropping
>> it in a follow-up cleanup patch.
>
> Do you plan to send it very recently.
> In that case I can rebase my patch on top of it.

It's not on my short term TODO.

---
Best Regards, Laurentiu