| Message ID | 20181130144743.675-1-miquel.raynal@bootlin.com |
|---|---|
| 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
Hello, This series adds a new driver to support Armada 3700 COMPHY IP. The series has been tested on an ESPRESSObin with SATA, PCIe and USB3 host. For this purpose, patch 1 enumerates the SATA PHY mode. The SGMII PHY mode that is supported by the IP has been written (uses SMC calls anyway) but could not be tested on this platform. Three series will follow to add PHY support (or at least a PHY nodes in the A3700/ESPRESSObin device trees) to the MVEBU AHCI driver, the Aardvark PCI controller driver and the MVEBU xHCI driver. All this is needed in order to achieve suspend to RAM on this platform. Thanks, Miquèl Changes since v1: ================= * Fix wrong check in ->xlate(). * Apply the same fix to the cp110 comphy driver from which the a3700 driver is based. * Added credit to Gregorz Jaszczyk for his work. * Added Suggested-by tag to the patch adding the COMPHY DT node. Grzegorz Jaszczyk (1): phy: enumerate SATA PHY mode Miquel Raynal (7): phy: mvebu-cp110-comphy: fix spelling in structure name phy: mvebu-cp110-comphy: fix port check in ->xlate() phy: add A3700 COMPHY support dt-bindings: phy: mvebu-comphy: extend the file to describe a3700 bindings MAINTAINERS: phy: add entry for Armada 3700 COMPHY driver ARM64: dts: marvell: armada-37xx: fix SATA node scope ARM64: dts: marvell: armada-37xx: declare the COMPHY node .../bindings/phy/phy-mvebu-comphy.txt | 65 ++++- MAINTAINERS | 6 + arch/arm64/boot/dts/marvell/armada-37xx.dtsi | 31 +- drivers/phy/marvell/Kconfig | 10 + drivers/phy/marvell/Makefile | 1 + drivers/phy/marvell/phy-mvebu-a3700-comphy.c | 276 ++++++++++++++++++ drivers/phy/marvell/phy-mvebu-cp110-comphy.c | 8 +- include/linux/phy/phy.h | 1 + 8 files changed, 381 insertions(+), 17 deletions(-) create mode 100644 drivers/phy/marvell/phy-mvebu-a3700-comphy.c