| Message ID | 20181015150147.23295-1-phil.edworthy@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | [v2] pinctrl: rzn1: Fix check for used MDIO bus | expand |
Hi Phil, On Mon, Oct 15, 2018 at 04:01:47PM +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)' > > It also fixes the return var when calling of_get_child_count() > Not really, since you skip the assignement if return value is <= 0: nfuncs = of_get_child_count(np); if (nfuncs <= 0) return 0; ipctl->nfunctions = nfuncs; This seems more likely to be here to make 'rzn1_pmx_get_funcs_count()' happy, as it returns a signed integer, but since nfunctions is only assigned if 'nfuncs' > 0, then the cast is safe there. I would keep this unsigned, and rather return an error if 'of_get_child_count()' returns an error of any sort in 'probe_dt()', otherwise assign 'ipctl->nfunctions = nfuncs;' unconditionally and return immediately if nfuncs == 0. This makes sure nfunctions is initialized even if there are no functions registered. Anyway, small issues, this just doesn't belong to this patch, but it's likely not to cause any harm I guess. > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> > --- > v2: > - Don't use implicit type conversion. > - Fix type of return var when calling of_get_child_count(). > --- > drivers/pinctrl/pinctrl-rzn1.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/pinctrl/pinctrl-rzn1.c b/drivers/pinctrl/pinctrl-rzn1.c > index ce05e3a00be2..4998463c54a0 100644 > --- a/drivers/pinctrl/pinctrl-rzn1.c > +++ b/drivers/pinctrl/pinctrl-rzn1.c > @@ -112,13 +112,13 @@ struct rzn1_pinctrl { > struct rzn1_pinctrl_regs __iomem *lev2; > u32 lev1_protect_phys; > u32 lev2_protect_phys; > - u32 mdio_func[2]; > + int mdio_func[2]; > > struct rzn1_pin_group *groups; > unsigned int ngroups; > > struct rzn1_pmx_func *functions; > - unsigned int nfunctions; > + int nfunctions; > }; > > #define RZN1_PINS_PROP "pinmux" > @@ -195,9 +195,9 @@ 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] >= 0 && ipctl->mdio_func[mdio] != (int)func) > dev_warn(ipctl->dev, "conflicting setting for mdio%d!\n", mdio); > - ipctl->mdio_func[mdio] = func; > + ipctl->mdio_func[mdio] = (int)func; > > dev_dbg(ipctl->dev, "setting mdio%d to %u\n", mdio, func); > > @@ -810,8 +810,8 @@ static int rzn1_pinctrl_probe_dt(struct platform_device *pdev, > struct device_node *np = pdev->dev.of_node; > struct device_node *child; > unsigned int maxgroups = 0; > - unsigned int nfuncs = 0; > unsigned int i = 0; > + int nfuncs = 0; > int ret; > > nfuncs = of_get_child_count(np); > -- > 2.17.1 >
Hi Jacopo, On 15 October 2018 16:12 jacopo mondi wrote: > On Mon, Oct 15, 2018 at 04:01:47PM +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)' > > > > It also fixes the return var when calling of_get_child_count() > > > > Not really, since you skip the assignement if return value is <= 0: > > nfuncs = of_get_child_count(np); > if (nfuncs <= 0) > return 0; > > ipctl->nfunctions = nfuncs; > > This seems more likely to be here to make 'rzn1_pmx_get_funcs_count()' > happy, as it returns a signed integer, but since nfunctions is only assigned if > 'nfuncs' > 0, then the cast is safe there. > > I would keep this unsigned, and rather return an error if > 'of_get_child_count()' returns an error of any sort in 'probe_dt()', otherwise > assign 'ipctl->nfunctions = nfuncs;' unconditionally and return immediately if > nfuncs == 0. This makes sure nfunctions is initialized even if there are no > functions registered. Ok, I get what you're saying, though nfunctions will always be initialised due to allocation with devm_kzalloc. > Anyway, small issues, this just doesn't belong to this patch, but it's likely not > to cause any harm I guess. True... should I leave this patch as is or respin again? Thanks Phil > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> > > --- > > v2: > > - Don't use implicit type conversion. > > - Fix type of return var when calling of_get_child_count(). > > --- > > drivers/pinctrl/pinctrl-rzn1.c | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/pinctrl/pinctrl-rzn1.c > > b/drivers/pinctrl/pinctrl-rzn1.c index ce05e3a00be2..4998463c54a0 > > 100644 > > --- a/drivers/pinctrl/pinctrl-rzn1.c > > +++ b/drivers/pinctrl/pinctrl-rzn1.c > > @@ -112,13 +112,13 @@ struct rzn1_pinctrl { > > struct rzn1_pinctrl_regs __iomem *lev2; > > u32 lev1_protect_phys; > > u32 lev2_protect_phys; > > - u32 mdio_func[2]; > > + int mdio_func[2]; > > > > struct rzn1_pin_group *groups; > > unsigned int ngroups; > > > > struct rzn1_pmx_func *functions; > > - unsigned int nfunctions; > > + int nfunctions; > > }; > > > > #define RZN1_PINS_PROP "pinmux" > > @@ -195,9 +195,9 @@ 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] >= 0 && ipctl->mdio_func[mdio] != > > +(int)func) > > dev_warn(ipctl->dev, "conflicting setting for mdio%d!\n", > mdio); > > - ipctl->mdio_func[mdio] = func; > > + ipctl->mdio_func[mdio] = (int)func; > > > > dev_dbg(ipctl->dev, "setting mdio%d to %u\n", mdio, func); > > > > @@ -810,8 +810,8 @@ static int rzn1_pinctrl_probe_dt(struct > platform_device *pdev, > > struct device_node *np = pdev->dev.of_node; > > struct device_node *child; > > unsigned int maxgroups = 0; > > - unsigned int nfuncs = 0; > > unsigned int i = 0; > > + int nfuncs = 0; > > int ret; > > > > nfuncs = of_get_child_count(np); > > -- > > 2.17.1 > >
Hi Phil, On Tue, Oct 16, 2018 at 08:04:53AM +0000, Phil Edworthy wrote: > Hi Jacopo, > > On 15 October 2018 16:12 jacopo mondi wrote: > > On Mon, Oct 15, 2018 at 04:01:47PM +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)' > > > > > > It also fixes the return var when calling of_get_child_count() > > > > > > > Not really, since you skip the assignement if return value is <= 0: > > > > nfuncs = of_get_child_count(np); > > if (nfuncs <= 0) > > return 0; > > > > ipctl->nfunctions = nfuncs; > > > > This seems more likely to be here to make 'rzn1_pmx_get_funcs_count()' > > happy, as it returns a signed integer, but since nfunctions is only assigned if > > 'nfuncs' > 0, then the cast is safe there. > > > > I would keep this unsigned, and rather return an error if > > 'of_get_child_count()' returns an error of any sort in 'probe_dt()', otherwise > > assign 'ipctl->nfunctions = nfuncs;' unconditionally and return immediately if > > nfuncs == 0. This makes sure nfunctions is initialized even if there are no > > functions registered. > Ok, I get what you're saying, though nfunctions will always be initialised due > to allocation with devm_kzalloc. > Right, sorry for missing that. > > Anyway, small issues, this just doesn't belong to this patch, but it's likely not > > to cause any harm I guess. > True... should I leave this patch as is or respin again? I think you should handle the case where of_get_child_count() returns an error, but in a follow up patch. You can leave this one as it is, even if that change is imho not necessary. Thanks j > > Thanks > Phil > > > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> > > > --- > > > v2: > > > - Don't use implicit type conversion. > > > - Fix type of return var when calling of_get_child_count(). > > > --- > > > drivers/pinctrl/pinctrl-rzn1.c | 10 +++++----- > > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/pinctrl/pinctrl-rzn1.c > > > b/drivers/pinctrl/pinctrl-rzn1.c index ce05e3a00be2..4998463c54a0 > > > 100644 > > > --- a/drivers/pinctrl/pinctrl-rzn1.c > > > +++ b/drivers/pinctrl/pinctrl-rzn1.c > > > @@ -112,13 +112,13 @@ struct rzn1_pinctrl { > > > struct rzn1_pinctrl_regs __iomem *lev2; > > > u32 lev1_protect_phys; > > > u32 lev2_protect_phys; > > > - u32 mdio_func[2]; > > > + int mdio_func[2]; > > > > > > struct rzn1_pin_group *groups; > > > unsigned int ngroups; > > > > > > struct rzn1_pmx_func *functions; > > > - unsigned int nfunctions; > > > + int nfunctions; > > > }; > > > > > > #define RZN1_PINS_PROP "pinmux" > > > @@ -195,9 +195,9 @@ 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] >= 0 && ipctl->mdio_func[mdio] != > > > +(int)func) > > > dev_warn(ipctl->dev, "conflicting setting for mdio%d!\n", > > mdio); > > > - ipctl->mdio_func[mdio] = func; > > > + ipctl->mdio_func[mdio] = (int)func; > > > > > > dev_dbg(ipctl->dev, "setting mdio%d to %u\n", mdio, func); > > > > > > @@ -810,8 +810,8 @@ static int rzn1_pinctrl_probe_dt(struct > > platform_device *pdev, > > > struct device_node *np = pdev->dev.of_node; > > > struct device_node *child; > > > unsigned int maxgroups = 0; > > > - unsigned int nfuncs = 0; > > > unsigned int i = 0; > > > + int nfuncs = 0; > > > int ret; > > > > > > nfuncs = of_get_child_count(np); > > > -- > > > 2.17.1 > > >
diff --git a/drivers/pinctrl/pinctrl-rzn1.c b/drivers/pinctrl/pinctrl-rzn1.c index ce05e3a00be2..4998463c54a0 100644 --- a/drivers/pinctrl/pinctrl-rzn1.c +++ b/drivers/pinctrl/pinctrl-rzn1.c @@ -112,13 +112,13 @@ struct rzn1_pinctrl { struct rzn1_pinctrl_regs __iomem *lev2; u32 lev1_protect_phys; u32 lev2_protect_phys; - u32 mdio_func[2]; + int mdio_func[2]; struct rzn1_pin_group *groups; unsigned int ngroups; struct rzn1_pmx_func *functions; - unsigned int nfunctions; + int nfunctions; }; #define RZN1_PINS_PROP "pinmux" @@ -195,9 +195,9 @@ 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] >= 0 && ipctl->mdio_func[mdio] != (int)func) dev_warn(ipctl->dev, "conflicting setting for mdio%d!\n", mdio); - ipctl->mdio_func[mdio] = func; + ipctl->mdio_func[mdio] = (int)func; dev_dbg(ipctl->dev, "setting mdio%d to %u\n", mdio, func); @@ -810,8 +810,8 @@ static int rzn1_pinctrl_probe_dt(struct platform_device *pdev, struct device_node *np = pdev->dev.of_node; struct device_node *child; unsigned int maxgroups = 0; - unsigned int nfuncs = 0; unsigned int i = 0; + int nfuncs = 0; int ret; nfuncs = of_get_child_count(np);
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)' It also fixes the return var when calling of_get_child_count() Reported-by: Dan Carpenter <dan.carpenter@oracle.com> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> --- v2: - Don't use implicit type conversion. - Fix type of return var when calling of_get_child_count(). --- drivers/pinctrl/pinctrl-rzn1.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)