[PATCH 0/3] mcb changes for 5.14

[PATCH 0/3] mcb changes for 5.14

[Johannes Thumshirn]

Hi Greg,

Here are the collected patches for mcb for 5.14.

The following changes since commit 009c9aa5be652675a06d5211e1640e02bbb1c33d:

  Linux 5.13-rc6 (2021-06-13 14:43:10 -0700)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/jth/linux.git mcb-for-5.14

for you to fetch changes up to fe520620eeddaa2ed8c669125f9b673c89d6b5a5:

  mcb: Remove trailing semicolon in macros (2021-06-15 23:43:47 +0900)

----------------------------------------------------------------

Alternatively the broken out patches are in this thread.

Dan Carpenter (1):
  mcb: fix error handling in mcb_alloc_bus()

Huilong Deng (1):
  mcb: Remove trailing semicolon in macros

Zhen Lei (1):
  mcb: Use DEFINE_RES_MEM() to simplify code

 drivers/mcb/mcb-core.c | 12 ++++++------
 drivers/mcb/mcb-lpc.c  | 13 ++-----------
 include/linux/mcb.h    |  2 +-
 3 files changed, 9 insertions(+), 18 deletions(-)

-- 
2.31.1

[PATCH 1/3] mcb: Use DEFINE_RES_MEM() to simplify code

[Johannes Thumshirn]

From: Zhen Lei <thunder.leizhen@huawei.com>

The value of '.end' should be "start + size - 1". So the previous writing
should have omitted subtracted 1.

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
Signed-off-by: Johannes Thumshirn <jth@kernel.org>
---
 drivers/mcb/mcb-lpc.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/mcb/mcb-lpc.c b/drivers/mcb/mcb-lpc.c
index 506676754538..53decd89876e 100644
--- a/drivers/mcb/mcb-lpc.c
+++ b/drivers/mcb/mcb-lpc.c
@@ -105,17 +105,8 @@ static int mcb_lpc_create_platform_device(const struct dmi_system_id *id)
 	return ret;
 }
 
-static struct resource sc24_fpga_resource = {
-	.start = 0xe000e000,
-	.end = 0xe000e000 + CHAM_HEADER_SIZE,
-	.flags = IORESOURCE_MEM,
-};
-
-static struct resource sc31_fpga_resource = {
-	.start = 0xf000e000,
-	.end = 0xf000e000 + CHAM_HEADER_SIZE,
-	.flags = IORESOURCE_MEM,
-};
+static struct resource sc24_fpga_resource = DEFINE_RES_MEM(0xe000e000, CHAM_HEADER_SIZE);
+static struct resource sc31_fpga_resource = DEFINE_RES_MEM(0xf000e000, CHAM_HEADER_SIZE);
 
 static struct platform_driver mcb_lpc_driver = {
 	.driver		= {
-- 
2.31.1

Re: [PATCH 1/3] mcb: Use DEFINE_RES_MEM() to simplify code

[Greg Kroah-Hartman]

On Tue, Jun 15, 2021 at 11:55:28PM +0900, Johannes Thumshirn wrote:
> From: Zhen Lei <thunder.leizhen@huawei.com>
> 
> The value of '.end' should be "start + size - 1". So the previous writing
> should have omitted subtracted 1.
> 
> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> Signed-off-by: Johannes Thumshirn <jth@kernel.org>
> ---
>  drivers/mcb/mcb-lpc.c | 13 ++-----------
>  1 file changed, 2 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/mcb/mcb-lpc.c b/drivers/mcb/mcb-lpc.c
> index 506676754538..53decd89876e 100644
> --- a/drivers/mcb/mcb-lpc.c
> +++ b/drivers/mcb/mcb-lpc.c
> @@ -105,17 +105,8 @@ static int mcb_lpc_create_platform_device(const struct dmi_system_id *id)
>  	return ret;
>  }
>  
> -static struct resource sc24_fpga_resource = {
> -	.start = 0xe000e000,
> -	.end = 0xe000e000 + CHAM_HEADER_SIZE,
> -	.flags = IORESOURCE_MEM,
> -};
> -
> -static struct resource sc31_fpga_resource = {
> -	.start = 0xf000e000,
> -	.end = 0xf000e000 + CHAM_HEADER_SIZE,
> -	.flags = IORESOURCE_MEM,
> -};
> +static struct resource sc24_fpga_resource = DEFINE_RES_MEM(0xe000e000, CHAM_HEADER_SIZE);
> +static struct resource sc31_fpga_resource = DEFINE_RES_MEM(0xf000e000, CHAM_HEADER_SIZE);

Does this simplify stuff, or does it fix a bug with the existing
definition?

Your changelog text says one thing, but your subject says the other...

thanks,

greg k-h

Re: [PATCH 1/3] mcb: Use DEFINE_RES_MEM() to simplify code

[Leizhen (ThunderTown)]

On 2021/6/15 23:42, Greg Kroah-Hartman wrote:
> On Tue, Jun 15, 2021 at 11:55:28PM +0900, Johannes Thumshirn wrote:
>> From: Zhen Lei <thunder.leizhen@huawei.com>
>>
>> The value of '.end' should be "start + size - 1". So the previous writing
>> should have omitted subtracted 1.
>>
>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>> Signed-off-by: Johannes Thumshirn <jth@kernel.org>
>> ---
>>  drivers/mcb/mcb-lpc.c | 13 ++-----------
>>  1 file changed, 2 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/mcb/mcb-lpc.c b/drivers/mcb/mcb-lpc.c
>> index 506676754538..53decd89876e 100644
>> --- a/drivers/mcb/mcb-lpc.c
>> +++ b/drivers/mcb/mcb-lpc.c
>> @@ -105,17 +105,8 @@ static int mcb_lpc_create_platform_device(const struct dmi_system_id *id)
>>  	return ret;
>>  }
>>  
>> -static struct resource sc24_fpga_resource = {
>> -	.start = 0xe000e000,
>> -	.end = 0xe000e000 + CHAM_HEADER_SIZE,
>> -	.flags = IORESOURCE_MEM,
>> -};
>> -
>> -static struct resource sc31_fpga_resource = {
>> -	.start = 0xf000e000,
>> -	.end = 0xf000e000 + CHAM_HEADER_SIZE,
>> -	.flags = IORESOURCE_MEM,
>> -};
>> +static struct resource sc24_fpga_resource = DEFINE_RES_MEM(0xe000e000, CHAM_HEADER_SIZE);
>> +static struct resource sc31_fpga_resource = DEFINE_RES_MEM(0xf000e000, CHAM_HEADER_SIZE);
> 
> Does this simplify stuff, or does it fix a bug with the existing
> definition?

It does not fix a bug, the actual value of .end should be "0xe000e000 + CHAM_HEADER_SIZE - 1".
There is no functional problem, just a little more memory. So it's just been corrected in the
process of simplification.

Do you think it's necessary to split it into two patches?

> 
> Your changelog text says one thing, but your subject says the other...

Okay, I'll change the description. How about:
Use DEFINE_RES_MEM() to save a couple of lines of code, which makes the
code a bit shorter and easier to read. The start address does not need to
appear twice.

By the way, the value of '.end' should be "start + size - 1". So the
previous writing should have omitted subtracted 1.

> 
> thanks,
> 
> greg k-h
> 
> .
> 

Re: [PATCH 1/3] mcb: Use DEFINE_RES_MEM() to simplify code

[Greg Kroah-Hartman]

On Wed, Jun 16, 2021 at 09:31:37AM +0800, Leizhen (ThunderTown) wrote:
> 
> 
> On 2021/6/15 23:42, Greg Kroah-Hartman wrote:
> > On Tue, Jun 15, 2021 at 11:55:28PM +0900, Johannes Thumshirn wrote:
> >> From: Zhen Lei <thunder.leizhen@huawei.com>
> >>
> >> The value of '.end' should be "start + size - 1". So the previous writing
> >> should have omitted subtracted 1.
> >>
> >> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> >> Signed-off-by: Johannes Thumshirn <jth@kernel.org>
> >> ---
> >>  drivers/mcb/mcb-lpc.c | 13 ++-----------
> >>  1 file changed, 2 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/drivers/mcb/mcb-lpc.c b/drivers/mcb/mcb-lpc.c
> >> index 506676754538..53decd89876e 100644
> >> --- a/drivers/mcb/mcb-lpc.c
> >> +++ b/drivers/mcb/mcb-lpc.c
> >> @@ -105,17 +105,8 @@ static int mcb_lpc_create_platform_device(const struct dmi_system_id *id)
> >>  	return ret;
> >>  }
> >>  
> >> -static struct resource sc24_fpga_resource = {
> >> -	.start = 0xe000e000,
> >> -	.end = 0xe000e000 + CHAM_HEADER_SIZE,
> >> -	.flags = IORESOURCE_MEM,
> >> -};
> >> -
> >> -static struct resource sc31_fpga_resource = {
> >> -	.start = 0xf000e000,
> >> -	.end = 0xf000e000 + CHAM_HEADER_SIZE,
> >> -	.flags = IORESOURCE_MEM,
> >> -};
> >> +static struct resource sc24_fpga_resource = DEFINE_RES_MEM(0xe000e000, CHAM_HEADER_SIZE);
> >> +static struct resource sc31_fpga_resource = DEFINE_RES_MEM(0xf000e000, CHAM_HEADER_SIZE);
> > 
> > Does this simplify stuff, or does it fix a bug with the existing
> > definition?
> 
> It does not fix a bug, the actual value of .end should be "0xe000e000 + CHAM_HEADER_SIZE - 1".
> There is no functional problem, just a little more memory. So it's just been corrected in the
> process of simplification.
> 
> Do you think it's necessary to split it into two patches?

No need, I just want to know if this is a fix that needs to go to older
kernels as well.  The text was very vague.


thanks,

greg k-h

[PATCH 2/3] mcb: fix error handling in mcb_alloc_bus()

[Johannes Thumshirn]

From: Dan Carpenter <dan.carpenter@oracle.com>

There are two bugs:
1) If ida_simple_get() fails then this code calls put_device(carrier)
   but we haven't yet called get_device(carrier) and probably that
   leads to a use after free.
2) After device_initialize() then we need to use put_device() to
   release the bus.  This will free the internal resources tied to the
   device and call mcb_free_bus() which will free the rest.

Fixes: 5d9e2ab9fea4 ("mcb: Implement bus->dev.release callback")
Fixes: 18d288198099 ("mcb: Correctly initialize the bus's device")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Johannes Thumshirn <jth@kernel.org>
---
 drivers/mcb/mcb-core.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/mcb/mcb-core.c b/drivers/mcb/mcb-core.c
index 38fbb3b59873..38cc8340e817 100644
--- a/drivers/mcb/mcb-core.c
+++ b/drivers/mcb/mcb-core.c
@@ -277,8 +277,8 @@ struct mcb_bus *mcb_alloc_bus(struct device *carrier)
 
 	bus_nr = ida_simple_get(&mcb_ida, 0, 0, GFP_KERNEL);
 	if (bus_nr < 0) {
-		rc = bus_nr;
-		goto err_free;
+		kfree(bus);
+		return ERR_PTR(bus_nr);
 	}
 
 	bus->bus_nr = bus_nr;
@@ -293,12 +293,12 @@ struct mcb_bus *mcb_alloc_bus(struct device *carrier)
 	dev_set_name(&bus->dev, "mcb:%d", bus_nr);
 	rc = device_add(&bus->dev);
 	if (rc)
-		goto err_free;
+		goto err_put;
 
 	return bus;
-err_free:
-	put_device(carrier);
-	kfree(bus);
+
+err_put:
+	put_device(&bus->dev);
 	return ERR_PTR(rc);
 }
 EXPORT_SYMBOL_NS_GPL(mcb_alloc_bus, MCB);
-- 
2.31.1

Re: [PATCH 2/3] mcb: fix error handling in mcb_alloc_bus()

[Greg Kroah-Hartman]

On Tue, Jun 15, 2021 at 11:55:29PM +0900, Johannes Thumshirn wrote:
> From: Dan Carpenter <dan.carpenter@oracle.com>
> 
> There are two bugs:
> 1) If ida_simple_get() fails then this code calls put_device(carrier)
>    but we haven't yet called get_device(carrier) and probably that
>    leads to a use after free.
> 2) After device_initialize() then we need to use put_device() to
>    release the bus.  This will free the internal resources tied to the
>    device and call mcb_free_bus() which will free the rest.
> 
> Fixes: 5d9e2ab9fea4 ("mcb: Implement bus->dev.release callback")
> Fixes: 18d288198099 ("mcb: Correctly initialize the bus's device")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Johannes Thumshirn <jth@kernel.org>
> ---
>  drivers/mcb/mcb-core.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)

Shouldn't this go to the stable kernels?  Why not cc: stable in the
signed-off-by lines?

thanks,

greg k-h

Re: [PATCH 2/3] mcb: fix error handling in mcb_alloc_bus()

[Johannes Thumshirn]

On 15/06/2021 17:42, Greg Kroah-Hartman wrote:
> On Tue, Jun 15, 2021 at 11:55:29PM +0900, Johannes Thumshirn wrote:
>> From: Dan Carpenter <dan.carpenter@oracle.com>
>>
>> There are two bugs:
>> 1) If ida_simple_get() fails then this code calls put_device(carrier)
>>    but we haven't yet called get_device(carrier) and probably that
>>    leads to a use after free.
>> 2) After device_initialize() then we need to use put_device() to
>>    release the bus.  This will free the internal resources tied to the
>>    device and call mcb_free_bus() which will free the rest.
>>
>> Fixes: 5d9e2ab9fea4 ("mcb: Implement bus->dev.release callback")
>> Fixes: 18d288198099 ("mcb: Correctly initialize the bus's device")
>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>> Signed-off-by: Johannes Thumshirn <jth@kernel.org>
>> ---
>>  drivers/mcb/mcb-core.c | 12 ++++++------
>>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> Shouldn't this go to the stable kernels?  Why not cc: stable in the
> signed-off-by lines?

Right, I'll add a cc stable

[PATCH 3/3] mcb: Remove trailing semicolon in macros

[Johannes Thumshirn]

From: Huilong Deng <denghuilong@cdjrlc.com>

Macros should not use a trailing semicolon.

Signed-off-by: Huilong Deng <denghuilong@cdjrlc.com>
Signed-off-by: Johannes Thumshirn <jth@kernel.org>
---
 include/linux/mcb.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/mcb.h b/include/linux/mcb.h
index 71dd10a3d928..f6efb16f9d1b 100644
--- a/include/linux/mcb.h
+++ b/include/linux/mcb.h
@@ -120,7 +120,7 @@ extern int __must_check __mcb_register_driver(struct mcb_driver *drv,
 	__mcb_register_driver(driver, THIS_MODULE, KBUILD_MODNAME)
 extern void mcb_unregister_driver(struct mcb_driver *driver);
 #define module_mcb_driver(__mcb_driver)		\
-	module_driver(__mcb_driver, mcb_register_driver, mcb_unregister_driver);
+	module_driver(__mcb_driver, mcb_register_driver, mcb_unregister_driver)
 extern void mcb_bus_add_devices(const struct mcb_bus *bus);
 extern int mcb_device_register(struct mcb_bus *bus, struct mcb_device *dev);
 extern struct mcb_bus *mcb_alloc_bus(struct device *carrier);
-- 
2.31.1

Re: [PATCH 3/3] mcb: Remove trailing semicolon in macros

[Greg Kroah-Hartman]

On Tue, Jun 15, 2021 at 11:55:30PM +0900, Johannes Thumshirn wrote:
> From: Huilong Deng <denghuilong@cdjrlc.com>
> 
> Macros should not use a trailing semicolon.
> 
> Signed-off-by: Huilong Deng <denghuilong@cdjrlc.com>
> Signed-off-by: Johannes Thumshirn <jth@kernel.org>
> ---
>  include/linux/mcb.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/mcb.h b/include/linux/mcb.h
> index 71dd10a3d928..f6efb16f9d1b 100644
> --- a/include/linux/mcb.h
> +++ b/include/linux/mcb.h
> @@ -120,7 +120,7 @@ extern int __must_check __mcb_register_driver(struct mcb_driver *drv,
>  	__mcb_register_driver(driver, THIS_MODULE, KBUILD_MODNAME)
>  extern void mcb_unregister_driver(struct mcb_driver *driver);
>  #define module_mcb_driver(__mcb_driver)		\
> -	module_driver(__mcb_driver, mcb_register_driver, mcb_unregister_driver);
> +	module_driver(__mcb_driver, mcb_register_driver, mcb_unregister_driver)
>  extern void mcb_bus_add_devices(const struct mcb_bus *bus);
>  extern int mcb_device_register(struct mcb_bus *bus, struct mcb_device *dev);
>  extern struct mcb_bus *mcb_alloc_bus(struct device *carrier);
> -- 
> 2.31.1
> 

I've applied this one, feel free to resend the first 2 as emails.

thanks,

greg k-h