| Message ID | 20181012104036.28829-1-phil.edworthy@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | pinctrl: rzn1: Fix check for used MDIO bus | expand |
Hi Phil, On Fri, Oct 12, 2018 at 11:40:36AM +0100, Phil Edworthy wrote: > This fixes the check for unused mdio bus setting and the following static > checker warning: > drivers/pinctrl/pinctrl-rzn1.c:198 rzn1_pinctrl_mdio_select() > warn: always true condition '(ipctl->mdio_func[mdio] >= 0) => (0-u32max >= 0)' > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> > --- > drivers/pinctrl/pinctrl-rzn1.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/pinctrl/pinctrl-rzn1.c b/drivers/pinctrl/pinctrl-rzn1.c > index ce05e3a00be2..f688f3a29dfd 100644 > --- a/drivers/pinctrl/pinctrl-rzn1.c > +++ b/drivers/pinctrl/pinctrl-rzn1.c > @@ -195,7 +195,7 @@ static void rzn1_hw_set_lock(struct rzn1_pinctrl *ipctl, u8 lock, u8 value) > static void rzn1_pinctrl_mdio_select(struct rzn1_pinctrl *ipctl, int mdio, > u32 func) > { > - if (ipctl->mdio_func[mdio] >= 0 && ipctl->mdio_func[mdio] != func) > + if (ipctl->mdio_func[mdio] != -1 && ipctl->mdio_func[mdio] != func) > dev_warn(ipctl->dev, "conflicting setting for mdio%d!\n", mdio); > ipctl->mdio_func[mdio] = func; > MY understanding here is that the static checker complains because you are comparing a variable of unsigned type to negative values, and indeed you are treating mdio_func as signed in the driver code. mdio_func is defined as: struct rzn1_pinctrl { ... u32 mdio_func[2]; ... }; Then in probe function mdio_func gets intialized as: ipctl->mdio_func[0] = -1; ipctl->mdio_func[1] = -1; I think you could safely make mdio_func integers, or either define an INVALID value ( > 0), intialize them it that value and check against it for validity. Thanks j > -- > 2.17.1 >
HI Jacopo, On 15 October 2018 08:54 jacopo mondi wrote: > On Fri, Oct 12, 2018 at 11:40:36AM +0100, Phil Edworthy wrote: > > This fixes the check for unused mdio bus setting and the following > > static checker warning: > > drivers/pinctrl/pinctrl-rzn1.c:198 rzn1_pinctrl_mdio_select() > > warn: always true condition '(ipctl->mdio_func[mdio] >= 0) => (0-u32max > >= 0)' > > > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> > > --- > > drivers/pinctrl/pinctrl-rzn1.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/pinctrl/pinctrl-rzn1.c > > b/drivers/pinctrl/pinctrl-rzn1.c index ce05e3a00be2..f688f3a29dfd > > 100644 > > --- a/drivers/pinctrl/pinctrl-rzn1.c > > +++ b/drivers/pinctrl/pinctrl-rzn1.c > > @@ -195,7 +195,7 @@ static void rzn1_hw_set_lock(struct rzn1_pinctrl > > *ipctl, u8 lock, u8 value) static void rzn1_pinctrl_mdio_select(struct > rzn1_pinctrl *ipctl, int mdio, > > u32 func) > > { > > - if (ipctl->mdio_func[mdio] >= 0 && ipctl->mdio_func[mdio] != func) > > + if (ipctl->mdio_func[mdio] != -1 && ipctl->mdio_func[mdio] != func) > > dev_warn(ipctl->dev, "conflicting setting for mdio%d!\n", > mdio); > > ipctl->mdio_func[mdio] = func; > > > > MY understanding here is that the static checker complains because you are > comparing a variable of unsigned type to negative values, and indeed you are > treating mdio_func as signed in the driver code. > > mdio_func is defined as: > > struct rzn1_pinctrl { > ... > u32 mdio_func[2]; > ... > }; > > Then in probe function mdio_func gets intialized as: > > ipctl->mdio_func[0] = -1; > ipctl->mdio_func[1] = -1; > > I think you could safely make mdio_func integers, or either define an > INVALID value ( > 0), intialize them it that value and check against it for > validity. You are right, I shouldn't rely on the implicit typecast from -1 to uint. I'll send an updated patch to fix this. Thanks Phil
diff --git a/drivers/pinctrl/pinctrl-rzn1.c b/drivers/pinctrl/pinctrl-rzn1.c index ce05e3a00be2..f688f3a29dfd 100644 --- a/drivers/pinctrl/pinctrl-rzn1.c +++ b/drivers/pinctrl/pinctrl-rzn1.c @@ -195,7 +195,7 @@ static void rzn1_hw_set_lock(struct rzn1_pinctrl *ipctl, u8 lock, u8 value) static void rzn1_pinctrl_mdio_select(struct rzn1_pinctrl *ipctl, int mdio, u32 func) { - if (ipctl->mdio_func[mdio] >= 0 && ipctl->mdio_func[mdio] != func) + if (ipctl->mdio_func[mdio] != -1 && ipctl->mdio_func[mdio] != func) dev_warn(ipctl->dev, "conflicting setting for mdio%d!\n", mdio); ipctl->mdio_func[mdio] = func;
This fixes the check for unused mdio bus setting and the following static checker warning: drivers/pinctrl/pinctrl-rzn1.c:198 rzn1_pinctrl_mdio_select() warn: always true condition '(ipctl->mdio_func[mdio] >= 0) => (0-u32max >= 0)' Reported-by: Dan Carpenter <dan.carpenter@oracle.com> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> --- drivers/pinctrl/pinctrl-rzn1.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)