diff mbox series

[v2,2/8] phy: mvebu-cp110-comphy: fix port check in ->xlate()

Message ID 20181130144743.675-3-miquel.raynal@bootlin.com (mailing list archive)
State New, archived
Headers show
Series Add Armada 3700 COMPHY support | expand

Commit Message

Miquel Raynal Nov. 30, 2018, 2:47 p.m. UTC
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(-)

Comments

Russell King - ARM Linux admin Nov. 30, 2018, 7 p.m. UTC | #1
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?
Miquel Raynal Dec. 2, 2018, 7:35 p.m. UTC | #2
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
Russell King - ARM Linux admin Dec. 3, 2018, 12:36 a.m. UTC | #3
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.
Miquel Raynal Dec. 3, 2018, 1:56 p.m. UTC | #4
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 mbox series

Patch

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;
 }