[PATCH net-next 0/2] dpaa2-eth: defer probe on object allocate

[PATCH net-next 0/2] dpaa2-eth: defer probe on object allocate

[Ioana Ciornei]

Allocatable objects on the fsl-mc bus may be probed by the fsl_mc_allocator
after the first attempts of other drivers to use them. Defer the probe when
this situation happens.

Ioana Ciornei (2):
  dpaa2-eth: defer probe on object allocate
  dpaa2-ptp: defer probe when portal allocation failed

 drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c | 30 +++++++++++++++++-------
 drivers/net/ethernet/freescale/dpaa2/dpaa2-ptp.c |  5 +++-
 2 files changed, 25 insertions(+), 10 deletions(-)

-- 
1.9.1

[PATCH net-next 1/2] dpaa2-eth: defer probe on object allocate

[Ioana Ciornei]

The fsl_mc_object_allocate function can fail because not all allocatable
objects are probed by the fsl_mc_allocator at the call time. Defer the
dpaa2-eth probe when this happens.

Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
 drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c | 30 +++++++++++++++++-------
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
index 88f7acc..71f5cd4 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
@@ -1434,8 +1434,11 @@ static struct fsl_mc_device *setup_dpcon(struct dpaa2_eth_priv *priv)
 	err = fsl_mc_object_allocate(to_fsl_mc_device(dev),
 				     FSL_MC_POOL_DPCON, &dpcon);
 	if (err) {
-		dev_info(dev, "Not enough DPCONs, will go on as-is\n");
-		return NULL;
+		if (err == -ENXIO)
+			err = -EPROBE_DEFER;
+		else
+			dev_info(dev, "Not enough DPCONs, will go on as-is\n");
+		return ERR_PTR(err);
 	}
 
 	err = dpcon_open(priv->mc_io, 0, dpcon->obj_desc.id, &dpcon->mc_handle);
@@ -1493,8 +1496,10 @@ static void free_dpcon(struct dpaa2_eth_priv *priv,
 		return NULL;
 
 	channel->dpcon = setup_dpcon(priv);
-	if (!channel->dpcon)
+	if (IS_ERR_OR_NULL(channel->dpcon)) {
+		err = PTR_ERR(channel->dpcon);
 		goto err_setup;
+	}
 
 	err = dpcon_get_attributes(priv->mc_io, 0, channel->dpcon->mc_handle,
 				   &attr);
@@ -1513,7 +1518,7 @@ static void free_dpcon(struct dpaa2_eth_priv *priv,
 	free_dpcon(priv, channel->dpcon);
 err_setup:
 	kfree(channel);
-	return NULL;
+	return ERR_PTR(err);
 }
 
 static void free_channel(struct dpaa2_eth_priv *priv,
@@ -1547,10 +1552,11 @@ static int setup_dpio(struct dpaa2_eth_priv *priv)
 	for_each_online_cpu(i) {
 		/* Try to allocate a channel */
 		channel = alloc_channel(priv);
-		if (!channel) {
-			dev_info(dev,
-				 "No affine channel for cpu %d and above\n", i);
-			err = -ENODEV;
+		if (IS_ERR_OR_NULL(channel)) {
+			err = PTR_ERR(channel);
+			if (err != -EPROBE_DEFER)
+				dev_info(dev,
+					 "No affine channel for cpu %d and above\n", i);
 			goto err_alloc_ch;
 		}
 
@@ -1608,6 +1614,9 @@ static int setup_dpio(struct dpaa2_eth_priv *priv)
 err_service_reg:
 	free_channel(priv, channel);
 err_alloc_ch:
+	if (err == -EPROBE_DEFER)
+		return err;
+
 	if (cpumask_empty(&priv->dpio_cpumask)) {
 		dev_err(dev, "No cpu with an affine DPIO/DPCON\n");
 		return err;
@@ -1732,7 +1741,10 @@ static int setup_dpbp(struct dpaa2_eth_priv *priv)
 	err = fsl_mc_object_allocate(to_fsl_mc_device(dev), FSL_MC_POOL_DPBP,
 				     &dpbp_dev);
 	if (err) {
-		dev_err(dev, "DPBP device allocation failed\n");
+		if (err == -ENXIO)
+			err = -EPROBE_DEFER;
+		else
+			dev_err(dev, "DPBP device allocation failed\n");
 		return err;
 	}
 
-- 
1.9.1

Re: [PATCH net-next 1/2] dpaa2-eth: defer probe on object allocate

[Andrew Lunn]

On Thu, Nov 08, 2018 at 01:17:47PM +0000, Ioana Ciornei wrote:
> The fsl_mc_object_allocate function can fail because not all allocatable
> objects are probed by the fsl_mc_allocator at the call time. Defer the
> dpaa2-eth probe when this happens.
> 
> Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> ---
>  drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c | 30 +++++++++++++++++-------
>  1 file changed, 21 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
> index 88f7acc..71f5cd4 100644
> --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
> +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
> @@ -1434,8 +1434,11 @@ static struct fsl_mc_device *setup_dpcon(struct dpaa2_eth_priv *priv)
>  	err = fsl_mc_object_allocate(to_fsl_mc_device(dev),
>  				     FSL_MC_POOL_DPCON, &dpcon);
>  	if (err) {
> -		dev_info(dev, "Not enough DPCONs, will go on as-is\n");
> -		return NULL;
> +		if (err == -ENXIO)
> +			err = -EPROBE_DEFER;
> +		else
> +			dev_info(dev, "Not enough DPCONs, will go on as-is\n");
> +		return ERR_PTR(err);
>  	}
>  
>  	err = dpcon_open(priv->mc_io, 0, dpcon->obj_desc.id, &dpcon->mc_handle);
> @@ -1493,8 +1496,10 @@ static void free_dpcon(struct dpaa2_eth_priv *priv,
>  		return NULL;
>  
>  	channel->dpcon = setup_dpcon(priv);
> -	if (!channel->dpcon)
> +	if (IS_ERR_OR_NULL(channel->dpcon)) {
> +		err = PTR_ERR(channel->dpcon);
>  		goto err_setup;
> +	}

Hi Ioana

You need to be careful with IS_ERR_OR_NULL(). If it is a NULL,
PTR_ERR() is going to return 0. You then jump to the error cleanup
code, but return 0, meaning everything is O.K.

      Andrew

RE: [PATCH net-next 1/2] dpaa2-eth: defer probe on object allocate

[Ioana Ciornei]

> > The fsl_mc_object_allocate function can fail because not all
> > allocatable objects are probed by the fsl_mc_allocator at the call
> > time. Defer the dpaa2-eth probe when this happens.
> >
> > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> > ---
> >  drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c | 30
> > +++++++++++++++++-------
> >  1 file changed, 21 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
> > b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
> > index 88f7acc..71f5cd4 100644
> > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
> > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
> > @@ -1434,8 +1434,11 @@ static struct fsl_mc_device *setup_dpcon(struct
> dpaa2_eth_priv *priv)
> >  	err = fsl_mc_object_allocate(to_fsl_mc_device(dev),
> >  				     FSL_MC_POOL_DPCON, &dpcon);
> >  	if (err) {
> > -		dev_info(dev, "Not enough DPCONs, will go on as-is\n");
> > -		return NULL;
> > +		if (err == -ENXIO)
> > +			err = -EPROBE_DEFER;
> > +		else
> > +			dev_info(dev, "Not enough DPCONs, will go on as-
> is\n");
> > +		return ERR_PTR(err);
> >  	}
> >
> >  	err = dpcon_open(priv->mc_io, 0, dpcon->obj_desc.id,
> > &dpcon->mc_handle); @@ -1493,8 +1496,10 @@ static void
> free_dpcon(struct dpaa2_eth_priv *priv,
> >  		return NULL;
> >
> >  	channel->dpcon = setup_dpcon(priv);
> > -	if (!channel->dpcon)
> > +	if (IS_ERR_OR_NULL(channel->dpcon)) {
> > +		err = PTR_ERR(channel->dpcon);
> >  		goto err_setup;
> > +	}
> 
> Hi Ioana
> 
> You need to be careful with IS_ERR_OR_NULL(). If it is a NULL,
> PTR_ERR() is going to return 0. You then jump to the error cleanup code, but
> return 0, meaning everything is O.K.
> 

Hi Andrew,

I took a closer look at the code path and it seems that if channel->dpcon in the snippet above is NULL,
then indeed PTR_ERR will be 0 but in the error cleanup code, in this case the err_setup label, 
a reverse ERR_PTR (NULL in this case) will be returned.
Continuing on the code path, alloc_channel then returns NULL and is handled by the following snippet.

+		if (IS_ERR_OR_NULL(channel)) {
+			err = PTR_ERR(channel);
+			if (err != -EPROBE_DEFER)
+				dev_info(dev,
+					 "No affine channel for cpu %d and above\n", i);
 			goto err_alloc_ch;
 		}
  
In case channel is NULL, then the dev_info will be called and the jump to the cleanup is made.

err_alloc_ch:
+	if (err == -EPROBE_DEFER)
+		return err;
+
 	if (cpumask_empty(&priv->dpio_cpumask)) {
 		dev_err(dev, "No cpu with an affine DPIO/DPCON\n");
 		return err;

Here err is 0 so in case the cpumask is empty, 0 will be returned, which is not the intended use.
I will send a v2 changing the return value to -ENODEV in case no cpus with an affine DPIO is found.

Thanks,
Ioana

Re: [PATCH net-next 1/2] dpaa2-eth: defer probe on object allocate

[Andrew Lunn]

Hi Ioana

> I will send a v2 changing the return value to -ENODEV in case no
> cpus with an affine DPIO is found.

It would be good to review all the cases where IS_ERR_OR_NULL() is
used. It is very easy to get wrong. In fact, it has been suggested
this macro is removed, because it is used wrongly way too often. Try
to avoid it if you can.

   Andrew

RE: [PATCH net-next 1/2] dpaa2-eth: defer probe on object allocate

[Ioana Ciornei]

> Hi Ioana
> 
> > I will send a v2 changing the return value to -ENODEV in case no cpus
> > with an affine DPIO is found.
> 
> It would be good to review all the cases where IS_ERR_OR_NULL() is used. It is
> very easy to get wrong. In fact, it has been suggested this macro is removed,
> because it is used wrongly way too often. Try to avoid it if you can.
> 

The macro is used just 2 times in the dpaa2-eth and both are part of the error path that I've reviewed and will update with a v2 of this patch.

Ioana  

[PATCH net-next 2/2] dpaa2-ptp: defer probe when portal allocation failed

[Ioana Ciornei]

The fsl_mc_portal_allocate can fail when the requested MC portals are
not yet probed by the fsl_mc_allocator. In this situation, the driver
should defer the probe.

Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
 drivers/net/ethernet/freescale/dpaa2/dpaa2-ptp.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-ptp.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-ptp.c
index 84b942b..9b150db 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-ptp.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-ptp.c
@@ -140,7 +140,10 @@ static int dpaa2_ptp_probe(struct fsl_mc_device *mc_dev)
 
 	err = fsl_mc_portal_allocate(mc_dev, 0, &mc_dev->mc_io);
 	if (err) {
-		dev_err(dev, "fsl_mc_portal_allocate err %d\n", err);
+		if (err == -ENXIO)
+			err = -EPROBE_DEFER;
+		else
+			dev_err(dev, "fsl_mc_portal_allocate err %d\n", err);
 		goto err_exit;
 	}
 
-- 
1.9.1

Re: [PATCH net-next 0/2] dpaa2-eth: defer probe on object allocate

[David Miller]

From: Ioana Ciornei <ioana.ciornei@nxp.com>
Date: Thu, 8 Nov 2018 13:17:46 +0000

> Allocatable objects on the fsl-mc bus may be probed by the fsl_mc_allocator
> after the first attempts of other drivers to use them. Defer the probe when
> this situation happens.

Series applied, thanks.

Re: [PATCH net-next 0/2] dpaa2-eth: defer probe on object allocate

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Thu, 08 Nov 2018 19:28:57 -0800 (PST)

> From: Ioana Ciornei <ioana.ciornei@nxp.com>
> Date: Thu, 8 Nov 2018 13:17:46 +0000
> 
>> Allocatable objects on the fsl-mc bus may be probed by the fsl_mc_allocator
>> after the first attempts of other drivers to use them. Defer the probe when
>> this situation happens.
> 
> Series applied, thanks.

Whoops, I just saw Andrew Lunn's feedback.

I reverted your changes, please address his concerns and resubmit.

Thank you.