| Message ID | 20181130144743.675-3-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Armada 3700 COMPHY support | expand |
On Fri, Nov 30, 2018 at 03:47:37PM +0100, Miquel Raynal wrote: > So far the PHY ->xlate() callback was checking if the port was > "invalid" before continuing, meaning that the port has not been used > yet. This check is not correct as there is no opposite call to > ->xlate() once the PHY is released by the user and the port will > remain "valid" after the first phy_get()/phy_put() calls. Hence, if > this driver is built as a module, inserted, removed and inserted > again, the PHY will appear busy and the second probe will fail. > > To fix this, just drop the faulty check and instead verify that the > port number is valid (ie. in the possible range). > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > drivers/phy/marvell/phy-mvebu-cp110-comphy.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/phy/marvell/phy-mvebu-cp110-comphy.c b/drivers/phy/marvell/phy-mvebu-cp110-comphy.c > index 31b9a1c18345..a40b876ff214 100644 > --- a/drivers/phy/marvell/phy-mvebu-cp110-comphy.c > +++ b/drivers/phy/marvell/phy-mvebu-cp110-comphy.c > @@ -567,9 +567,9 @@ static struct phy *mvebu_comphy_xlate(struct device *dev, > return phy; > > lane = phy_get_drvdata(phy); > - if (lane->port >= 0) > - return ERR_PTR(-EBUSY); > lane->port = args->args[0]; > + if (lane->port >= MVEBU_COMPHY_PORTS) > + return ERR_PTR(-EINVAL); Shouldn't we validate args->args[0] before doing anything?
Hi Russell, Russell King - ARM Linux <linux@armlinux.org.uk> wrote on Fri, 30 Nov 2018 19:00:31 +0000: > On Fri, Nov 30, 2018 at 03:47:37PM +0100, Miquel Raynal wrote: > > So far the PHY ->xlate() callback was checking if the port was > > "invalid" before continuing, meaning that the port has not been used > > yet. This check is not correct as there is no opposite call to > > ->xlate() once the PHY is released by the user and the port will > > remain "valid" after the first phy_get()/phy_put() calls. Hence, if > > this driver is built as a module, inserted, removed and inserted > > again, the PHY will appear busy and the second probe will fail. > > > > To fix this, just drop the faulty check and instead verify that the > > port number is valid (ie. in the possible range). > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > --- > > drivers/phy/marvell/phy-mvebu-cp110-comphy.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/phy/marvell/phy-mvebu-cp110-comphy.c b/drivers/phy/marvell/phy-mvebu-cp110-comphy.c > > index 31b9a1c18345..a40b876ff214 100644 > > --- a/drivers/phy/marvell/phy-mvebu-cp110-comphy.c > > +++ b/drivers/phy/marvell/phy-mvebu-cp110-comphy.c > > @@ -567,9 +567,9 @@ static struct phy *mvebu_comphy_xlate(struct device *dev, > > return phy; > > > > lane = phy_get_drvdata(phy); > > - if (lane->port >= 0) > > - return ERR_PTR(-EBUSY); > > lane->port = args->args[0]; > > + if (lane->port >= MVEBU_COMPHY_PORTS) > > + return ERR_PTR(-EINVAL); > > Shouldn't we validate args->args[0] before doing anything? > I don't understand your point, there is a check on args->args[0] as we check its value (through lane->port) right after. What do you have in mind? Thanks, Miquèl
On Sun, Dec 02, 2018 at 08:35:09PM +0100, Miquel Raynal wrote: > Hi Russell, > > Russell King - ARM Linux <linux@armlinux.org.uk> wrote on Fri, 30 Nov > 2018 19:00:31 +0000: > > > On Fri, Nov 30, 2018 at 03:47:37PM +0100, Miquel Raynal wrote: > > > So far the PHY ->xlate() callback was checking if the port was > > > "invalid" before continuing, meaning that the port has not been used > > > yet. This check is not correct as there is no opposite call to > > > ->xlate() once the PHY is released by the user and the port will > > > remain "valid" after the first phy_get()/phy_put() calls. Hence, if > > > this driver is built as a module, inserted, removed and inserted > > > again, the PHY will appear busy and the second probe will fail. > > > > > > To fix this, just drop the faulty check and instead verify that the > > > port number is valid (ie. in the possible range). > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > --- > > > drivers/phy/marvell/phy-mvebu-cp110-comphy.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/phy/marvell/phy-mvebu-cp110-comphy.c b/drivers/phy/marvell/phy-mvebu-cp110-comphy.c > > > index 31b9a1c18345..a40b876ff214 100644 > > > --- a/drivers/phy/marvell/phy-mvebu-cp110-comphy.c > > > +++ b/drivers/phy/marvell/phy-mvebu-cp110-comphy.c > > > @@ -567,9 +567,9 @@ static struct phy *mvebu_comphy_xlate(struct device *dev, > > > return phy; > > > > > > lane = phy_get_drvdata(phy); > > > - if (lane->port >= 0) > > > - return ERR_PTR(-EBUSY); > > > lane->port = args->args[0]; > > > + if (lane->port >= MVEBU_COMPHY_PORTS) > > > + return ERR_PTR(-EINVAL); > > > > Shouldn't we validate args->args[0] before doing anything? > > > > I don't understand your point, there is a check on args->args[0] as > we check its value (through lane->port) right after. What do you > have in mind? Right, there is already a check on args->args[0] for it being greater than MVEBU_COMPHY_PORTS and returning an error (and in fact warning if that is the case). So in that case, what is the use of the above additional test you are proposing to add? The resulting code ends up looking like this: if (WARN_ON(args->args[0] >= MVEBU_COMPHY_PORTS)) return ERR_PTR(-EINVAL); ... lane->port = args->args[0]; + if (lane->port >= MVEBU_COMPHY_PORTS) + return ERR_PTR(-EINVAL); which is just silly - the second test can never be evaluated as true, and therefore is redundant. In any case, my point was that in your patch, where you assign lane->port and then validate the lane->port value, this is in principle the wrong order - the order should always be: validate first, then make use.
Hi Russell, Russell King - ARM Linux <linux@armlinux.org.uk> wrote on Mon, 3 Dec 2018 00:36:23 +0000: > On Sun, Dec 02, 2018 at 08:35:09PM +0100, Miquel Raynal wrote: > > Hi Russell, > > > > Russell King - ARM Linux <linux@armlinux.org.uk> wrote on Fri, 30 Nov > > 2018 19:00:31 +0000: > > > > > On Fri, Nov 30, 2018 at 03:47:37PM +0100, Miquel Raynal wrote: > > > > So far the PHY ->xlate() callback was checking if the port was > > > > "invalid" before continuing, meaning that the port has not been used > > > > yet. This check is not correct as there is no opposite call to > > > > ->xlate() once the PHY is released by the user and the port will > > > > remain "valid" after the first phy_get()/phy_put() calls. Hence, if > > > > this driver is built as a module, inserted, removed and inserted > > > > again, the PHY will appear busy and the second probe will fail. > > > > > > > > To fix this, just drop the faulty check and instead verify that the > > > > port number is valid (ie. in the possible range). > > > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > > --- > > > > drivers/phy/marvell/phy-mvebu-cp110-comphy.c | 4 ++-- > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/phy/marvell/phy-mvebu-cp110-comphy.c b/drivers/phy/marvell/phy-mvebu-cp110-comphy.c > > > > index 31b9a1c18345..a40b876ff214 100644 > > > > --- a/drivers/phy/marvell/phy-mvebu-cp110-comphy.c > > > > +++ b/drivers/phy/marvell/phy-mvebu-cp110-comphy.c > > > > @@ -567,9 +567,9 @@ static struct phy *mvebu_comphy_xlate(struct device *dev, > > > > return phy; > > > > > > > > lane = phy_get_drvdata(phy); > > > > - if (lane->port >= 0) > > > > - return ERR_PTR(-EBUSY); > > > > lane->port = args->args[0]; > > > > + if (lane->port >= MVEBU_COMPHY_PORTS) > > > > + return ERR_PTR(-EINVAL); > > > > > > Shouldn't we validate args->args[0] before doing anything? > > > > > > > I don't understand your point, there is a check on args->args[0] as > > we check its value (through lane->port) right after. What do you > > have in mind? > > Right, there is already a check on args->args[0] for it being greater > than MVEBU_COMPHY_PORTS and returning an error (and in fact warning > if that is the case). So in that case, what is the use of the above > additional test you are proposing to add? The resulting code ends up > looking like this: > > if (WARN_ON(args->args[0] >= MVEBU_COMPHY_PORTS)) > return ERR_PTR(-EINVAL); > ... > lane->port = args->args[0]; > + if (lane->port >= MVEBU_COMPHY_PORTS) > + return ERR_PTR(-EINVAL); > > which is just silly - the second test can never be evaluated as true, > and therefore is redundant. > > In any case, my point was that in your patch, where you assign > lane->port and then validate the lane->port value, this is in > principle the wrong order - the order should always be: validate > first, then make use. > You are right, this test is redundant; I forgot about the first check. I will just drop these additional two lines and just do: [...] lane->port = args->args[0]; return 0; } Thanks, Miquèl
diff --git a/drivers/phy/marvell/phy-mvebu-cp110-comphy.c b/drivers/phy/marvell/phy-mvebu-cp110-comphy.c index 31b9a1c18345..a40b876ff214 100644 --- a/drivers/phy/marvell/phy-mvebu-cp110-comphy.c +++ b/drivers/phy/marvell/phy-mvebu-cp110-comphy.c @@ -567,9 +567,9 @@ static struct phy *mvebu_comphy_xlate(struct device *dev, return phy; lane = phy_get_drvdata(phy); - if (lane->port >= 0) - return ERR_PTR(-EBUSY); lane->port = args->args[0]; + if (lane->port >= MVEBU_COMPHY_PORTS) + return ERR_PTR(-EINVAL); return phy; }
So far the PHY ->xlate() callback was checking if the port was "invalid" before continuing, meaning that the port has not been used yet. This check is not correct as there is no opposite call to ->xlate() once the PHY is released by the user and the port will remain "valid" after the first phy_get()/phy_put() calls. Hence, if this driver is built as a module, inserted, removed and inserted again, the PHY will appear busy and the second probe will fail. To fix this, just drop the faulty check and instead verify that the port number is valid (ie. in the possible range). Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- drivers/phy/marvell/phy-mvebu-cp110-comphy.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)