[PATCH v1 1/1] mfd: intel_quark_i2c_gpio: Don't play dirty trick with const

[PATCH v1 1/1] mfd: intel_quark_i2c_gpio: Don't play dirty trick with const

[Andy Shevchenko]

As Linus rightfully noticed, the driver plays dirty trick with const,
i.e. it assigns a place holder data structure to the const field
in the MFD cell and then drops the const by explicit casting. This
is not how it should be.

Replace cell parameter by bar and assign local pointer res to the
respective non-const place holder in the intel_quark_i2c_setup()
and intel_quark_gpio_setup().

Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/mfd/intel_quark_i2c_gpio.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/drivers/mfd/intel_quark_i2c_gpio.c b/drivers/mfd/intel_quark_i2c_gpio.c
index 7f90e6f022ba..f2bce2ade510 100644
--- a/drivers/mfd/intel_quark_i2c_gpio.c
+++ b/drivers/mfd/intel_quark_i2c_gpio.c
@@ -157,17 +157,16 @@ static void intel_quark_unregister_i2c_clk(struct device *dev)
 	clk_unregister(quark_mfd->i2c_clk);
 }
 
-static int intel_quark_i2c_setup(struct pci_dev *pdev, struct mfd_cell *cell)
+static int intel_quark_i2c_setup(struct pci_dev *pdev, int bar)
 {
+	struct mfd_cell *cell = &intel_quark_mfd_cells[bar];
+	struct resource *res = intel_quark_i2c_res;
 	const struct dmi_system_id *dmi_id;
 	struct dw_i2c_platform_data *pdata;
-	struct resource *res = (struct resource *)cell->resources;
 	struct device *dev = &pdev->dev;
 
-	res[INTEL_QUARK_IORES_MEM].start =
-		pci_resource_start(pdev, MFD_I2C_BAR);
-	res[INTEL_QUARK_IORES_MEM].end =
-		pci_resource_end(pdev, MFD_I2C_BAR);
+	res[INTEL_QUARK_IORES_MEM].start = pci_resource_start(pdev, bar);
+	res[INTEL_QUARK_IORES_MEM].end = pci_resource_end(pdev, bar);
 
 	res[INTEL_QUARK_IORES_IRQ].start = pci_irq_vector(pdev, 0);
 	res[INTEL_QUARK_IORES_IRQ].end = pci_irq_vector(pdev, 0);
@@ -189,16 +188,15 @@ static int intel_quark_i2c_setup(struct pci_dev *pdev, struct mfd_cell *cell)
 	return 0;
 }
 
-static int intel_quark_gpio_setup(struct pci_dev *pdev, struct mfd_cell *cell)
+static int intel_quark_gpio_setup(struct pci_dev *pdev, int bar)
 {
+	struct mfd_cell *cell = &intel_quark_mfd_cells[bar];
+	struct resource *res = intel_quark_gpio_res;
 	struct dwapb_platform_data *pdata;
-	struct resource *res = (struct resource *)cell->resources;
 	struct device *dev = &pdev->dev;
 
-	res[INTEL_QUARK_IORES_MEM].start =
-		pci_resource_start(pdev, MFD_GPIO_BAR);
-	res[INTEL_QUARK_IORES_MEM].end =
-		pci_resource_end(pdev, MFD_GPIO_BAR);
+	res[INTEL_QUARK_IORES_MEM].start = pci_resource_start(pdev, bar);
+	res[INTEL_QUARK_IORES_MEM].end = pci_resource_end(pdev, bar);
 
 	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
 	if (!pdata)
@@ -252,11 +250,11 @@ static int intel_quark_mfd_probe(struct pci_dev *pdev,
 	if (ret < 0)
 		goto err_unregister_i2c_clk;
 
-	ret = intel_quark_i2c_setup(pdev, &intel_quark_mfd_cells[MFD_I2C_BAR]);
+	ret = intel_quark_i2c_setup(pdev, MFD_I2C_BAR);
 	if (ret)
 		goto err_free_irq_vectors;
 
-	ret = intel_quark_gpio_setup(pdev, &intel_quark_mfd_cells[MFD_GPIO_BAR]);
+	ret = intel_quark_gpio_setup(pdev, MFD_GPIO_BAR);
 	if (ret)
 		goto err_free_irq_vectors;
 
-- 
2.30.2

Re: [PATCH v1 1/1] mfd: intel_quark_i2c_gpio: Don't play dirty trick with const

[Linus Torvalds]

On Thu, Mar 25, 2021 at 12:23 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> Replace cell parameter by bar and assign local pointer res to the
> respective non-const place holder in the intel_quark_i2c_setup()
> and intel_quark_gpio_setup().

Thanks. I assume/hope this got tested on hardware too? It looks
ObviouslyCorrect(tm), but that's never stopped bugs before ;)

             Linus

Re: [PATCH v1 1/1] mfd: intel_quark_i2c_gpio: Don't play dirty trick with const

[Andy Shevchenko]

On Thu, Mar 25, 2021 at 9:31 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Thu, Mar 25, 2021 at 12:23 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > Replace cell parameter by bar and assign local pointer res to the
> > respective non-const place holder in the intel_quark_i2c_setup()
> > and intel_quark_gpio_setup().
>
> Thanks. I assume/hope this got tested on hardware too?

Yes. On exact hardware this driver is for.

>  It looks
> ObviouslyCorrect(tm), but that's never stopped bugs before ;)


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] mfd: intel_quark_i2c_gpio: Don't play dirty trick with const

[Lee Jones]

On Thu, 25 Mar 2021, Andy Shevchenko wrote:

> As Linus rightfully noticed, the driver plays dirty trick with const,
> i.e. it assigns a place holder data structure to the const field
> in the MFD cell and then drops the const by explicit casting. This
> is not how it should be.
> 
> Replace cell parameter by bar and assign local pointer res to the
> respective non-const place holder in the intel_quark_i2c_setup()
> and intel_quark_gpio_setup().
> 
> Reported-by: Linus Torvalds <torvalds@linux-foundation.org>

Thanks for spotting this Linus.

> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/mfd/intel_quark_i2c_gpio.c | 26 ++++++++++++--------------
>  1 file changed, 12 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/mfd/intel_quark_i2c_gpio.c b/drivers/mfd/intel_quark_i2c_gpio.c
> index 7f90e6f022ba..f2bce2ade510 100644
> --- a/drivers/mfd/intel_quark_i2c_gpio.c
> +++ b/drivers/mfd/intel_quark_i2c_gpio.c
> @@ -157,17 +157,16 @@ static void intel_quark_unregister_i2c_clk(struct device *dev)
>  	clk_unregister(quark_mfd->i2c_clk);
>  }
>  
> -static int intel_quark_i2c_setup(struct pci_dev *pdev, struct mfd_cell *cell)
> +static int intel_quark_i2c_setup(struct pci_dev *pdev, int bar)
>  {
> +	struct mfd_cell *cell = &intel_quark_mfd_cells[bar];

What's stopping you from removing the seemingly superfluous 'bar'
parameter and simply doing:

	struct mfd_cell *cell = &intel_quark_mfd_cells[MFD_I2C_BAR];	  

> +	struct resource *res = intel_quark_i2c_res;
>  	const struct dmi_system_id *dmi_id;
>  	struct dw_i2c_platform_data *pdata;
> -	struct resource *res = (struct resource *)cell->resources;
>  	struct device *dev = &pdev->dev;
>  
> -	res[INTEL_QUARK_IORES_MEM].start =
> -		pci_resource_start(pdev, MFD_I2C_BAR);
> -	res[INTEL_QUARK_IORES_MEM].end =
> -		pci_resource_end(pdev, MFD_I2C_BAR);
> +	res[INTEL_QUARK_IORES_MEM].start = pci_resource_start(pdev, bar);
> +	res[INTEL_QUARK_IORES_MEM].end = pci_resource_end(pdev, bar);

If you do end up using 'MFD_I2C_BAR' again in here, please omit the
line wrap.  80-char lines are so 1990s!

>  	res[INTEL_QUARK_IORES_IRQ].start = pci_irq_vector(pdev, 0);
>  	res[INTEL_QUARK_IORES_IRQ].end = pci_irq_vector(pdev, 0);
> @@ -189,16 +188,15 @@ static int intel_quark_i2c_setup(struct pci_dev *pdev, struct mfd_cell *cell)
>  	return 0;
>  }

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog