| Message ID | 1573568693-18642-1-git-send-email-ioana.ciornei@nxp.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net,v2] dpaa2-eth: free already allocated channels on probe defer | expand |
On Tue, Nov 12, 2019 at 04:24:53PM +0200, Ioana Ciornei wrote: > The setup_dpio() function tries to allocate a number of channels equal > to the number of CPUs online. When there are not enough DPCON objects > already probed, the function will return EPROBE_DEFER. When this > happens, the already allocated channels are not freed. This results in > the incapacity of properly probing the next time around. > Fix this by freeing the channels on the error path. > > Fixes: d7f5a9d89a55 ("dpaa2-eth: defer probe on object allocate") Its not clear to me that this clean-up problem was added by the defer change. But rather, looking at the git logs, it seems likely to have been present since the driver was added by: 6e2387e8f19e ("staging: fsl-dpaa2/eth: Add Freescale DPAA2 Ethernet driver") > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com> > --- > Changes in v2: > - add the proper Fixes tag > > drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > index 19379bae0144..22e9519f65bb 100644 > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > @@ -2232,6 +2232,14 @@ static int setup_dpio(struct dpaa2_eth_priv *priv) > err_service_reg: > free_channel(priv, channel); > err_alloc_ch: > + for (i = 0; i < priv->num_channels; i++) { > + channel = priv->channel[i]; > + nctx = &channel->nctx; > + dpaa2_io_service_deregister(channel->dpio, nctx, dev); > + free_channel(priv, channel); > + } > + priv->num_channels = 0; > + > if (err == -EPROBE_DEFER) > return err; This function goes on to return 0 unless cpumask_empty(&priv->dpio_cpumask) is zero. Given this is an errorr path and the clean-up above, is that correct? > > -- > 1.9.1 >
> On Tue, Nov 12, 2019 at 04:24:53PM +0200, Ioana Ciornei wrote: > > The setup_dpio() function tries to allocate a number of channels equal > > to the number of CPUs online. When there are not enough DPCON objects > > already probed, the function will return EPROBE_DEFER. When this > > happens, the already allocated channels are not freed. This results in > > the incapacity of properly probing the next time around. > > Fix this by freeing the channels on the error path. > > > > Fixes: d7f5a9d89a55 ("dpaa2-eth: defer probe on object allocate") > > Its not clear to me that this clean-up problem was added by the defer change. > But rather, looking at the git logs, it seems likely to have been present since the > driver was added by: > > 6e2387e8f19e ("staging: fsl-dpaa2/eth: Add Freescale DPAA2 Ethernet driver") > The problem is not present in the initial driver. The logic before the d7f5a9d89a55 ("dpaa2-eth: defer probe on object allocate") patch was that if there are not enough channels available it will go ahead with less than the optimal value. > > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com> > > --- > > Changes in v2: > > - add the proper Fixes tag > > > > drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > > b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > > index 19379bae0144..22e9519f65bb 100644 > > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > > @@ -2232,6 +2232,14 @@ static int setup_dpio(struct dpaa2_eth_priv > > *priv) > > err_service_reg: > > free_channel(priv, channel); > > err_alloc_ch: > > + for (i = 0; i < priv->num_channels; i++) { > > + channel = priv->channel[i]; > > + nctx = &channel->nctx; > > + dpaa2_io_service_deregister(channel->dpio, nctx, dev); > > + free_channel(priv, channel); > > + } > > + priv->num_channels = 0; > > + > > if (err == -EPROBE_DEFER) > > return err; > > This function goes on to return 0 unless cpumask_empty(&priv->dpio_cpumask) > is zero. Given this is an errorr path and the clean-up above, is that correct? You're right, cleaning up the channels should be done just on the EPROBE_DEFER path, in the if statement, and not for the entire code path. I'll rework this. Thanks for pointing this out. Ioana > > > > > -- > > 1.9.1 > >
diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c index 19379bae0144..22e9519f65bb 100644 --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c @@ -2232,6 +2232,14 @@ static int setup_dpio(struct dpaa2_eth_priv *priv) err_service_reg: free_channel(priv, channel); err_alloc_ch: + for (i = 0; i < priv->num_channels; i++) { + channel = priv->channel[i]; + nctx = &channel->nctx; + dpaa2_io_service_deregister(channel->dpio, nctx, dev); + free_channel(priv, channel); + } + priv->num_channels = 0; + if (err == -EPROBE_DEFER) return err;
The setup_dpio() function tries to allocate a number of channels equal to the number of CPUs online. When there are not enough DPCON objects already probed, the function will return EPROBE_DEFER. When this happens, the already allocated channels are not freed. This results in the incapacity of properly probing the next time around. Fix this by freeing the channels on the error path. Fixes: d7f5a9d89a55 ("dpaa2-eth: defer probe on object allocate") Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com> --- Changes in v2: - add the proper Fixes tag drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c | 8 ++++++++ 1 file changed, 8 insertions(+)