| Message ID | 20170301153625.16249-1-p.zabel@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Philipp, On Wed, Mar 01, 2017 at 04:36:25PM +0100, Philipp Zabel wrote: > Recently, an unfinished patch was merged that added a third entry to the > beginning of the array of firmware locations without changing the code > to also look at the third element, thus pushing an old firmware location > off the list. > > Fixes: 8af7779f3cbc ("[media] coda: add Freescale firmware compatibility location") > Cc: Baruch Siach <baruch@tkos.co.il> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> Thanks for cleaning up after me. Acked-by: Baruch Siach <baruch@tkos.co.il> [snip] > - if (dev->firmware == 1) { > + if (dev->firmware > 0) { > /* > * Since we can't suppress warnings for failed asynchronous > * firmware requests, report that the fallback firmware was I still think this is ugly, though. baruch
On Wed, Mar 1, 2017 at 12:36 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote: > Recently, an unfinished patch was merged that added a third entry to the > beginning of the array of firmware locations without changing the code > to also look at the third element, thus pushing an old firmware location > off the list. > > Fixes: 8af7779f3cbc ("[media] coda: add Freescale firmware compatibility location") > Cc: Baruch Siach <baruch@tkos.co.il> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> Reviewed-by: Fabio Estevam <fabio.estevam@nxp.com>
On 01/03/17 16:36, Philipp Zabel wrote: > Recently, an unfinished patch was merged that added a third entry to the > beginning of the array of firmware locations without changing the code > to also look at the third element, thus pushing an old firmware location > off the list. > > Fixes: 8af7779f3cbc ("[media] coda: add Freescale firmware compatibility location") > Cc: Baruch Siach <baruch@tkos.co.il> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > --- > drivers/media/platform/coda/coda-common.c | 17 ++++++++++------- > 1 file changed, 10 insertions(+), 7 deletions(-) > > diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c > index eb6548f46cbac..e1a2e8c70db01 100644 > --- a/drivers/media/platform/coda/coda-common.c > +++ b/drivers/media/platform/coda/coda-common.c > @@ -2128,6 +2128,9 @@ static int coda_firmware_request(struct coda_dev *dev) > { > char *fw = dev->devtype->firmware[dev->firmware]; > > + if (dev->firmware >= ARRAY_SIZE(dev->devtype->firmware)) > + return -EINVAL; > + Move the fw assignment after this 'if'. Otherwise it's reading from undefined memory if dev->firmware >= ARRAY_SIZE(dev->devtype->firmware). Regards, Hans > dev_dbg(&dev->plat_dev->dev, "requesting firmware '%s' for %s\n", fw, > coda_product_name(dev->devtype->product)); > > @@ -2142,16 +2145,16 @@ static void coda_fw_callback(const struct firmware *fw, void *context) > struct platform_device *pdev = dev->plat_dev; > int i, ret; > > - if (!fw && dev->firmware == 1) { > - v4l2_err(&dev->v4l2_dev, "firmware request failed\n"); > - goto put_pm; > - } > if (!fw) { > - dev->firmware = 1; > - coda_firmware_request(dev); > + dev->firmware++; > + ret = coda_firmware_request(dev); > + if (ret < 0) { > + v4l2_err(&dev->v4l2_dev, "firmware request failed\n"); > + goto put_pm; > + } > return; > } > - if (dev->firmware == 1) { > + if (dev->firmware > 0) { > /* > * Since we can't suppress warnings for failed asynchronous > * firmware requests, report that the fallback firmware was >
On Wed, 2017-03-08 at 11:38 +0100, Hans Verkuil wrote: > On 01/03/17 16:36, Philipp Zabel wrote: > > Recently, an unfinished patch was merged that added a third entry to the > > beginning of the array of firmware locations without changing the code > > to also look at the third element, thus pushing an old firmware location > > off the list. > > > > Fixes: 8af7779f3cbc ("[media] coda: add Freescale firmware compatibility location") > > Cc: Baruch Siach <baruch@tkos.co.il> > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > > --- > > drivers/media/platform/coda/coda-common.c | 17 ++++++++++------- > > 1 file changed, 10 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c > > index eb6548f46cbac..e1a2e8c70db01 100644 > > --- a/drivers/media/platform/coda/coda-common.c > > +++ b/drivers/media/platform/coda/coda-common.c > > @@ -2128,6 +2128,9 @@ static int coda_firmware_request(struct coda_dev *dev) > > { > > char *fw = dev->devtype->firmware[dev->firmware]; > > > > + if (dev->firmware >= ARRAY_SIZE(dev->devtype->firmware)) > > + return -EINVAL; > > + > > Move the fw assignment after this 'if'. Otherwise it's reading from undefined memory > if dev->firmware >= ARRAY_SIZE(dev->devtype->firmware). > > Regards, > > Hans Will do, thanks for the review. regards Philipp
diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c index eb6548f46cbac..e1a2e8c70db01 100644 --- a/drivers/media/platform/coda/coda-common.c +++ b/drivers/media/platform/coda/coda-common.c @@ -2128,6 +2128,9 @@ static int coda_firmware_request(struct coda_dev *dev) { char *fw = dev->devtype->firmware[dev->firmware]; + if (dev->firmware >= ARRAY_SIZE(dev->devtype->firmware)) + return -EINVAL; + dev_dbg(&dev->plat_dev->dev, "requesting firmware '%s' for %s\n", fw, coda_product_name(dev->devtype->product)); @@ -2142,16 +2145,16 @@ static void coda_fw_callback(const struct firmware *fw, void *context) struct platform_device *pdev = dev->plat_dev; int i, ret; - if (!fw && dev->firmware == 1) { - v4l2_err(&dev->v4l2_dev, "firmware request failed\n"); - goto put_pm; - } if (!fw) { - dev->firmware = 1; - coda_firmware_request(dev); + dev->firmware++; + ret = coda_firmware_request(dev); + if (ret < 0) { + v4l2_err(&dev->v4l2_dev, "firmware request failed\n"); + goto put_pm; + } return; } - if (dev->firmware == 1) { + if (dev->firmware > 0) { /* * Since we can't suppress warnings for failed asynchronous * firmware requests, report that the fallback firmware was
Recently, an unfinished patch was merged that added a third entry to the beginning of the array of firmware locations without changing the code to also look at the third element, thus pushing an old firmware location off the list. Fixes: 8af7779f3cbc ("[media] coda: add Freescale firmware compatibility location") Cc: Baruch Siach <baruch@tkos.co.il> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> --- drivers/media/platform/coda/coda-common.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)