| Message ID | 20190912204450.17625-1-christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: v4l: cadence: Fix how unsued lanes are handled in 'csi2rx_start()' | expand |
Am 12.09.2019 22:44, schrieb Christophe JAILLET: > The 2nd parameter of 'find_first_zero_bit()' is a number of bits, not of > bytes. So use 'BITS_PER_LONG' instead of 'sizeof(lanes_used)'. > > Fixes: 1fc3b37f34f6 ("media: v4l: cadence: Add Cadence MIPI-CSI2 RX driver") > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > --- > This patch is purely speculative. Using BITS_PER_LONG looks logical to me, > but I'm not 100% sure that it is what is expected here. 'csi2rx->max_lanes' > could also be a good candidate. > --- > drivers/media/platform/cadence/cdns-csi2rx.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c > index 31ace114eda1..28765ccb1b12 100644 > --- a/drivers/media/platform/cadence/cdns-csi2rx.c > +++ b/drivers/media/platform/cadence/cdns-csi2rx.c > @@ -129,7 +129,7 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx) > */ > for (i = csi2rx->num_lanes; i < csi2rx->max_lanes; i++) { > unsigned int idx = find_first_zero_bit(&lanes_used, > - sizeof(lanes_used)); > + BITS_PER_LONG); why not CHAR_BIT*sizeof(lanes_used) ? this would have the advantage that it is independent of future changes of lanes_used. re, wh > set_bit(idx, &lanes_used); > reg |= CSI2RX_STATIC_CFG_DLANE_MAP(i, i + 1); > }
Hi Christophe, On Thu, Sep 12, 2019 at 10:44:50PM +0200, Christophe JAILLET wrote: > The 2nd parameter of 'find_first_zero_bit()' is a number of bits, not of > bytes. So use 'BITS_PER_LONG' instead of 'sizeof(lanes_used)'. > > Fixes: 1fc3b37f34f6 ("media: v4l: cadence: Add Cadence MIPI-CSI2 RX driver") > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > --- > This patch is purely speculative. Using BITS_PER_LONG looks logical to me, > but I'm not 100% sure that it is what is expected here. 'csi2rx->max_lanes' > could also be a good candidate. Yeah, csi2rx->max_lanes would make more sense in that context. Could you resend a new version? Thanks! Maxime
On Fri, Sep 13, 2019 at 09:57:09AM +0200, Maxime Ripard wrote: > Hi Christophe, > > On Thu, Sep 12, 2019 at 10:44:50PM +0200, Christophe JAILLET wrote: > > The 2nd parameter of 'find_first_zero_bit()' is a number of bits, not of > > bytes. So use 'BITS_PER_LONG' instead of 'sizeof(lanes_used)'. > > > > Fixes: 1fc3b37f34f6 ("media: v4l: cadence: Add Cadence MIPI-CSI2 RX driver") > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > > --- > > This patch is purely speculative. Using BITS_PER_LONG looks logical to me, > > but I'm not 100% sure that it is what is expected here. 'csi2rx->max_lanes' > > could also be a good candidate. > > Yeah, csi2rx->max_lanes would make more sense in that context. Could > you resend a new version? This is sort of unrelated, but for Smatch purposes the csi2rx->max_lanes comes from the firmware in csi2rx_parse_dt() and it could be any u8 value. I sort of wish that people would write code which was known to be correct just from reading the kernel code, without looking at the firmware... I guess I could mark v4l2_fwnode_endpoint_parse() as always giving us valid data, but that still wouldn't tell us what the valid data is. It's hard to know the right answer from a static analysis point of view. regards, dan carpenter
Le 16/09/2019 à 08:28, Dan Carpenter a écrit : > On Fri, Sep 13, 2019 at 09:57:09AM +0200, Maxime Ripard wrote: >> Hi Christophe, >> >> On Thu, Sep 12, 2019 at 10:44:50PM +0200, Christophe JAILLET wrote: >>> The 2nd parameter of 'find_first_zero_bit()' is a number of bits, not of >>> bytes. So use 'BITS_PER_LONG' instead of 'sizeof(lanes_used)'. >>> >>> Fixes: 1fc3b37f34f6 ("media: v4l: cadence: Add Cadence MIPI-CSI2 RX driver") >>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> >>> --- >>> This patch is purely speculative. Using BITS_PER_LONG looks logical to me, >>> but I'm not 100% sure that it is what is expected here. 'csi2rx->max_lanes' >>> could also be a good candidate. >> Yeah, csi2rx->max_lanes would make more sense in that context. Could >> you resend a new version? > This is sort of unrelated, but for Smatch purposes the csi2rx->max_lanes > comes from the firmware in csi2rx_parse_dt() and it could be any u8 > value. Hi Dan, not sure to follow you. csi2rx_probe() --> csi2rx_get_resources() --> ... dev_cfg = readl(csi2rx->base + CSI2RX_DEVICE_CFG_REG); ... csi2rx->max_lanes = dev_cfg & 7; if (csi2rx->max_lanes > CSI2RX_LANES_MAX) { dev_err(&pdev->dev, "Invalid number of lanes: %u\n", csi2rx->max_lanes); return -EINVAL; } So I guess, that we can trust max_lanes because of the 'if (... > CSI2RX_LANES_MAX)' check. Did I miss something? > I sort of wish that people would write code which was known to be > correct just from reading the kernel code, without looking at the > firmware... I guess I could mark v4l2_fwnode_endpoint_parse() as always > giving us valid data, but that still wouldn't tell us what the valid > data is. It's hard to know the right answer from a static analysis > point of view. > > regards, > dan carpenter > >
On Mon, Sep 16, 2019 at 09:24:26PM +0200, Christophe JAILLET wrote: > Le 16/09/2019 à 08:28, Dan Carpenter a écrit : > > On Fri, Sep 13, 2019 at 09:57:09AM +0200, Maxime Ripard wrote: > > > Hi Christophe, > > > > > > On Thu, Sep 12, 2019 at 10:44:50PM +0200, Christophe JAILLET wrote: > > > > The 2nd parameter of 'find_first_zero_bit()' is a number of bits, not of > > > > bytes. So use 'BITS_PER_LONG' instead of 'sizeof(lanes_used)'. > > > > > > > > Fixes: 1fc3b37f34f6 ("media: v4l: cadence: Add Cadence MIPI-CSI2 RX driver") > > > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > > > > --- > > > > This patch is purely speculative. Using BITS_PER_LONG looks logical to me, > > > > but I'm not 100% sure that it is what is expected here. 'csi2rx->max_lanes' > > > > could also be a good candidate. > > > Yeah, csi2rx->max_lanes would make more sense in that context. Could > > > you resend a new version? > > This is sort of unrelated, but for Smatch purposes the csi2rx->max_lanes > > comes from the firmware in csi2rx_parse_dt() and it could be any u8 > > value. > > Hi Dan, > > not sure to follow you. > > csi2rx_probe() > --> csi2rx_get_resources() > --> ... > dev_cfg = readl(csi2rx->base + CSI2RX_DEVICE_CFG_REG); > ... > csi2rx->max_lanes = dev_cfg & 7; > if (csi2rx->max_lanes > CSI2RX_LANES_MAX) { > dev_err(&pdev->dev, "Invalid number of lanes: %u\n", > csi2rx->max_lanes); > return -EINVAL; > } > > So I guess, that we can trust max_lanes because of the 'if (... > > CSI2RX_LANES_MAX)' check. > > Did I miss something? Ugh... I was looking at ->num_lanes and I was also just totally wrong. Smatch parses that badly. Smatch actually parses ->max_lanes correctly though so that's ok. Sorry for the noise on this. regards, dan carpenter
On 12/09/2019 22:44, Christophe JAILLET wrote: > Subject: [PATCH] media: v4l: cadence: Fix how unsued lanes are handled in 'csi2rx_start()' s/unsued/unused > The 2nd parameter of 'find_first_zero_bit()' is a number of bits, not of > bytes. So use 'BITS_PER_LONG' instead of 'sizeof(lanes_used)'. > > Fixes: 1fc3b37f34f6 ("media: v4l: cadence: Add Cadence MIPI-CSI2 RX driver") > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c index 31ace114eda1..28765ccb1b12 100644 --- a/drivers/media/platform/cadence/cdns-csi2rx.c +++ b/drivers/media/platform/cadence/cdns-csi2rx.c @@ -129,7 +129,7 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx) */ for (i = csi2rx->num_lanes; i < csi2rx->max_lanes; i++) { unsigned int idx = find_first_zero_bit(&lanes_used, - sizeof(lanes_used)); + BITS_PER_LONG); set_bit(idx, &lanes_used); reg |= CSI2RX_STATIC_CFG_DLANE_MAP(i, i + 1); }
The 2nd parameter of 'find_first_zero_bit()' is a number of bits, not of bytes. So use 'BITS_PER_LONG' instead of 'sizeof(lanes_used)'. Fixes: 1fc3b37f34f6 ("media: v4l: cadence: Add Cadence MIPI-CSI2 RX driver") Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- This patch is purely speculative. Using BITS_PER_LONG looks logical to me, but I'm not 100% sure that it is what is expected here. 'csi2rx->max_lanes' could also be a good candidate. --- drivers/media/platform/cadence/cdns-csi2rx.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)