[PATCH 0/2] PS3: Adjustments for six function implementations

[PATCH 0/2] PS3: Adjustments for six function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 16 Dec 2017 14:42:24 +0100

Two update suggestions were taken into account
from static source code analysis.

Markus Elfring (2):
  Delete an error message for a failed memory allocation in two functions
  Improve a size determination in five functions

 drivers/ps3/ps3-lpm.c         |  2 --
 drivers/ps3/ps3-sys-manager.c |  9 ++-------
 drivers/ps3/ps3-vuart.c       | 12 +++---------
 3 files changed, 5 insertions(+), 18 deletions(-)

-- 
2.15.1

[PATCH 0/2] PS3: Adjustments for six function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 16 Dec 2017 14:42:24 +0100

Two update suggestions were taken into account
from static source code analysis.

Markus Elfring (2):
  Delete an error message for a failed memory allocation in two functions
  Improve a size determination in five functions

 drivers/ps3/ps3-lpm.c         |  2 --
 drivers/ps3/ps3-sys-manager.c |  9 ++-------
 drivers/ps3/ps3-vuart.c       | 12 +++---------
 3 files changed, 5 insertions(+), 18 deletions(-)

-- 
2.15.1

[PATCH 1/2] ps3: Delete an error message for a failed memory allocation in two functions

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 16 Dec 2017 12:32:42 +0100

Omit an extra message for a memory allocation failure in these functions.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/ps3/ps3-lpm.c   | 2 --
 drivers/ps3/ps3-vuart.c | 1 -
 2 files changed, 3 deletions(-)

diff --git a/drivers/ps3/ps3-lpm.c b/drivers/ps3/ps3-lpm.c
index e34de9a7d517..ac8d5725c9b4 100644
--- a/drivers/ps3/ps3-lpm.c
+++ b/drivers/ps3/ps3-lpm.c
@@ -1123,8 +1123,6 @@ int ps3_lpm_open(enum ps3_lpm_tb_type tb_type, void *tb_cache,
 		lpm_priv->tb_cache_internal = kzalloc(
 			lpm_priv->tb_cache_size + 127, GFP_KERNEL);
 		if (!lpm_priv->tb_cache_internal) {
-			dev_err(sbd_core(), "%s:%u: alloc internal tb_cache "
-				"failed\n", __func__, __LINE__);
 			result = -ENOMEM;
 			goto fail_malloc;
 		}
diff --git a/drivers/ps3/ps3-vuart.c b/drivers/ps3/ps3-vuart.c
index b7f300b79ffd..bbaad30a876f 100644
--- a/drivers/ps3/ps3-vuart.c
+++ b/drivers/ps3/ps3-vuart.c
@@ -929,7 +929,6 @@ static int ps3_vuart_bus_interrupt_get(void)
 	vuart_bus_priv.bmp = kzalloc(sizeof(struct ports_bmp), GFP_KERNEL);
 
 	if (!vuart_bus_priv.bmp) {
-		pr_debug("%s:%d: kzalloc failed.\n", __func__, __LINE__);
 		result = -ENOMEM;
 		goto fail_bmp_malloc;
 	}
-- 
2.15.1

[PATCH 1/2] ps3: Delete an error message for a failed memory allocation in two functions

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 16 Dec 2017 12:32:42 +0100

Omit an extra message for a memory allocation failure in these functions.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/ps3/ps3-lpm.c   | 2 --
 drivers/ps3/ps3-vuart.c | 1 -
 2 files changed, 3 deletions(-)

diff --git a/drivers/ps3/ps3-lpm.c b/drivers/ps3/ps3-lpm.c
index e34de9a7d517..ac8d5725c9b4 100644
--- a/drivers/ps3/ps3-lpm.c
+++ b/drivers/ps3/ps3-lpm.c
@@ -1123,8 +1123,6 @@ int ps3_lpm_open(enum ps3_lpm_tb_type tb_type, void *tb_cache,
 		lpm_priv->tb_cache_internal = kzalloc(
 			lpm_priv->tb_cache_size + 127, GFP_KERNEL);
 		if (!lpm_priv->tb_cache_internal) {
-			dev_err(sbd_core(), "%s:%u: alloc internal tb_cache "
-				"failed\n", __func__, __LINE__);
 			result = -ENOMEM;
 			goto fail_malloc;
 		}
diff --git a/drivers/ps3/ps3-vuart.c b/drivers/ps3/ps3-vuart.c
index b7f300b79ffd..bbaad30a876f 100644
--- a/drivers/ps3/ps3-vuart.c
+++ b/drivers/ps3/ps3-vuart.c
@@ -929,7 +929,6 @@ static int ps3_vuart_bus_interrupt_get(void)
 	vuart_bus_priv.bmp = kzalloc(sizeof(struct ports_bmp), GFP_KERNEL);
 
 	if (!vuart_bus_priv.bmp) {
-		pr_debug("%s:%d: kzalloc failed.\n", __func__, __LINE__);
 		result = -ENOMEM;
 		goto fail_bmp_malloc;
 	}
-- 
2.15.1

Re: [PATCH 1/2] ps3: Delete an error message for a failed memory allocation in two functions

[Geoff Levand]

On 12/16/2017 05:51 AM, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 16 Dec 2017 12:32:42 +0100
> 
> Omit an extra message for a memory allocation failure in these functions.

This is OK, I'll add it to my ps3-queue branch.

-Geoff

Re: [PATCH 1/2] ps3: Delete an error message for a failed memory allocation in two functions

[Geoff Levand]

On 12/16/2017 05:51 AM, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 16 Dec 2017 12:32:42 +0100
> 
> Omit an extra message for a memory allocation failure in these functions.

This is OK, I'll add it to my ps3-queue branch.

-Geoff

[PATCH 2/2] ps3: Improve a size determination in five functions

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 16 Dec 2017 14:21:04 +0100

Replace the specification of data structures by variable references
as the parameter for the operator "sizeof" to make the corresponding size
determination a bit safer according to the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/ps3/ps3-sys-manager.c |  9 ++-------
 drivers/ps3/ps3-vuart.c       | 11 +++--------
 2 files changed, 5 insertions(+), 15 deletions(-)

diff --git a/drivers/ps3/ps3-sys-manager.c b/drivers/ps3/ps3-sys-manager.c
index 73e496a72113..e7d8ef93576a 100644
--- a/drivers/ps3/ps3-sys-manager.c
+++ b/drivers/ps3/ps3-sys-manager.c
@@ -249,9 +249,7 @@ static int ps3_sys_manager_write(struct ps3_system_bus_device *dev,
 	BUG_ON(header->payload_size != 8 && header->payload_size != 16);
 	BUG_ON(header->service_id > 8);
 
-	result = ps3_vuart_write(dev, header,
-		sizeof(struct ps3_sys_manager_header));
-
+	result = ps3_vuart_write(dev, header, sizeof(*header));
 	if (!result)
 		result = ps3_vuart_write(dev, payload, header->payload_size);
 
@@ -537,11 +535,8 @@ static int ps3_sys_manager_handle_cmd(struct ps3_system_bus_device *dev)
 
 static int ps3_sys_manager_handle_msg(struct ps3_system_bus_device *dev)
 {
-	int result;
 	struct ps3_sys_manager_header header;
-
-	result = ps3_vuart_read(dev, &header,
-		sizeof(struct ps3_sys_manager_header));
+	int result = ps3_vuart_read(dev, &header, sizeof(header));
 
 	if (result)
 		return result;
diff --git a/drivers/ps3/ps3-vuart.c b/drivers/ps3/ps3-vuart.c
index bbaad30a876f..c82362cbaf4a 100644
--- a/drivers/ps3/ps3-vuart.c
+++ b/drivers/ps3/ps3-vuart.c
@@ -522,8 +522,7 @@ int ps3_vuart_write(struct ps3_system_bus_device *dev, const void *buf,
 	} else
 		spin_unlock_irqrestore(&priv->tx_list.lock, flags);
 
-	lb = kmalloc(sizeof(struct list_buffer) + bytes, GFP_KERNEL);
-
+	lb = kmalloc(sizeof(*lb) + bytes, GFP_KERNEL);
 	if (!lb)
 		return -ENOMEM;
 
@@ -575,9 +574,7 @@ static int ps3_vuart_queue_rx_bytes(struct ps3_system_bus_device *dev,
 	/* Add some extra space for recently arrived data. */
 
 	bytes += 128;
-
-	lb = kmalloc(sizeof(struct list_buffer) + bytes, GFP_ATOMIC);
-
+	lb = kmalloc(sizeof(*lb) + bytes, GFP_ATOMIC);
 	if (!lb)
 		return -ENOMEM;
 
@@ -925,9 +922,7 @@ static int ps3_vuart_bus_interrupt_get(void)
 		return 0;
 
 	BUG_ON(vuart_bus_priv.bmp);
-
-	vuart_bus_priv.bmp = kzalloc(sizeof(struct ports_bmp), GFP_KERNEL);
-
+	vuart_bus_priv.bmp = kzalloc(sizeof(*vuart_bus_priv.bmp), GFP_KERNEL);
 	if (!vuart_bus_priv.bmp) {
 		result = -ENOMEM;
 		goto fail_bmp_malloc;
-- 
2.15.1

[PATCH 2/2] ps3: Improve a size determination in five functions

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 16 Dec 2017 14:21:04 +0100

Replace the specification of data structures by variable references
as the parameter for the operator "sizeof" to make the corresponding size
determination a bit safer according to the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/ps3/ps3-sys-manager.c |  9 ++-------
 drivers/ps3/ps3-vuart.c       | 11 +++--------
 2 files changed, 5 insertions(+), 15 deletions(-)

diff --git a/drivers/ps3/ps3-sys-manager.c b/drivers/ps3/ps3-sys-manager.c
index 73e496a72113..e7d8ef93576a 100644
--- a/drivers/ps3/ps3-sys-manager.c
+++ b/drivers/ps3/ps3-sys-manager.c
@@ -249,9 +249,7 @@ static int ps3_sys_manager_write(struct ps3_system_bus_device *dev,
 	BUG_ON(header->payload_size != 8 && header->payload_size != 16);
 	BUG_ON(header->service_id > 8);
 
-	result = ps3_vuart_write(dev, header,
-		sizeof(struct ps3_sys_manager_header));
-
+	result = ps3_vuart_write(dev, header, sizeof(*header));
 	if (!result)
 		result = ps3_vuart_write(dev, payload, header->payload_size);
 
@@ -537,11 +535,8 @@ static int ps3_sys_manager_handle_cmd(struct ps3_system_bus_device *dev)
 
 static int ps3_sys_manager_handle_msg(struct ps3_system_bus_device *dev)
 {
-	int result;
 	struct ps3_sys_manager_header header;
-
-	result = ps3_vuart_read(dev, &header,
-		sizeof(struct ps3_sys_manager_header));
+	int result = ps3_vuart_read(dev, &header, sizeof(header));
 
 	if (result)
 		return result;
diff --git a/drivers/ps3/ps3-vuart.c b/drivers/ps3/ps3-vuart.c
index bbaad30a876f..c82362cbaf4a 100644
--- a/drivers/ps3/ps3-vuart.c
+++ b/drivers/ps3/ps3-vuart.c
@@ -522,8 +522,7 @@ int ps3_vuart_write(struct ps3_system_bus_device *dev, const void *buf,
 	} else
 		spin_unlock_irqrestore(&priv->tx_list.lock, flags);
 
-	lb = kmalloc(sizeof(struct list_buffer) + bytes, GFP_KERNEL);
-
+	lb = kmalloc(sizeof(*lb) + bytes, GFP_KERNEL);
 	if (!lb)
 		return -ENOMEM;
 
@@ -575,9 +574,7 @@ static int ps3_vuart_queue_rx_bytes(struct ps3_system_bus_device *dev,
 	/* Add some extra space for recently arrived data. */
 
 	bytes += 128;
-
-	lb = kmalloc(sizeof(struct list_buffer) + bytes, GFP_ATOMIC);
-
+	lb = kmalloc(sizeof(*lb) + bytes, GFP_ATOMIC);
 	if (!lb)
 		return -ENOMEM;
 
@@ -925,9 +922,7 @@ static int ps3_vuart_bus_interrupt_get(void)
 		return 0;
 
 	BUG_ON(vuart_bus_priv.bmp);
-
-	vuart_bus_priv.bmp = kzalloc(sizeof(struct ports_bmp), GFP_KERNEL);
-
+	vuart_bus_priv.bmp = kzalloc(sizeof(*vuart_bus_priv.bmp), GFP_KERNEL);
 	if (!vuart_bus_priv.bmp) {
 		result = -ENOMEM;
 		goto fail_bmp_malloc;
-- 
2.15.1

Re: [PATCH 2/2] ps3: Improve a size determination in five functions

[Geoff Levand]

Hi,

On 12/16/2017 05:54 AM, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 16 Dec 2017 14:21:04 +0100
> 
> Replace the specification of data structures by variable references
> as the parameter for the operator "sizeof" to make the corresponding size
> determination a bit safer according to the Linux coding style convention.

This would be OK, but you are also removing empty lines and changing
the coding format.

Please update the patch to only make the sizeof changes.

-Geoff

Re: [PATCH 2/2] ps3: Improve a size determination in five functions

[Geoff Levand]

Hi,

On 12/16/2017 05:54 AM, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 16 Dec 2017 14:21:04 +0100
> 
> Replace the specification of data structures by variable references
> as the parameter for the operator "sizeof" to make the corresponding size
> determination a bit safer according to the Linux coding style convention.

This would be OK, but you are also removing empty lines and changing
the coding format.

Please update the patch to only make the sizeof changes.

-Geoff

Re: ps3: Improve a size determination in five functions

[SF Markus Elfring]

>> Replace the specification of data structures by variable references
>> as the parameter for the operator "sizeof" to make the corresponding size
>> determination a bit safer according to the Linux coding style convention.
> 
> This would be OK,

Thanks.


> but you are also removing empty lines and changing the coding format.

* Why do you not like such a change combination?

* Do you really want to integrate further update steps in this case?

Regards,
Markus

Re: ps3: Improve a size determination in five functions

[SF Markus Elfring]

>> Replace the specification of data structures by variable references
>> as the parameter for the operator "sizeof" to make the corresponding size
>> determination a bit safer according to the Linux coding style convention.
> 
> This would be OK,

Thanks.


> but you are also removing empty lines and changing the coding format.

* Why do you not like such a change combination?

* Do you really want to integrate further update steps in this case?

Regards,
Markus

Re: ps3: Improve a size determination in five functions

[Geoff Levand]

On 12/18/2017 12:45 PM, SF Markus Elfring wrote:
>> but you are also removing empty lines and changing the coding format.
> 
> * Why do you not like such a change combination?
> 
> * Do you really want to integrate further update steps in this case?

I want to keep the coding style consistent throughout the file.

-Geoff

Re: ps3: Improve a size determination in five functions

[Geoff Levand]

On 12/18/2017 12:45 PM, SF Markus Elfring wrote:
>> but you are also removing empty lines and changing the coding format.
> 
> * Why do you not like such a change combination?
> 
> * Do you really want to integrate further update steps in this case?

I want to keep the coding style consistent throughout the file.

-Geoff

Re: [PATCH 2/2] ps3: Improve a size determination in five functions

[Geoff Levand]

Hi,

On 12/16/2017 05:54 AM, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 16 Dec 2017 14:21:04 +0100
> 
> Replace the specification of data structures by variable references
> as the parameter for the operator "sizeof" to make the corresponding size
> determination a bit safer according to the Linux coding style convention.

After some thought, I've decided to reject this patch and others like
it because I feel it will make long term maintenance of the PS3 code
more difficult.

Some observations:

 o Your patch fixes no bug nor replaces any depreciated feature.
 o There will be no functional change; the generated binary
   will be nearly identical.
 o The PS3 kernel support is now over 10 years old.
 o I need to continue support for a few old kernel versions,
   specifically linux-3.15 and linux-2.6.30.  That includes
   keeping them working with new toolchain versions.  I need
   to back port fixes to these old kernels.
 o When problems arise I sometimes need to use git bisect
   back to old kernel versions.  When I do the bisect I often
   have fixes and local debug patches that I apply to the
   bisected tree before building.
 o Source code changes between versions causes patch conflicts
   that need to be manually resolved.  This can be error prone
   and very time consuming on a long bisect session.

My decision to reject this patch and others like it is in
attempt to minimize the code maintenance effort.  If you have
patches that fix bugs, upgrade depreciated features, or
generally improve functionality please submit them for
review.

-Geoff

Re: [PATCH 2/2] ps3: Improve a size determination in five functions

[Geoff Levand]

Hi,

On 12/16/2017 05:54 AM, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 16 Dec 2017 14:21:04 +0100
> 
> Replace the specification of data structures by variable references
> as the parameter for the operator "sizeof" to make the corresponding size
> determination a bit safer according to the Linux coding style convention.

After some thought, I've decided to reject this patch and others like
it because I feel it will make long term maintenance of the PS3 code
more difficult.

Some observations:

 o Your patch fixes no bug nor replaces any depreciated feature.
 o There will be no functional change; the generated binary
   will be nearly identical.
 o The PS3 kernel support is now over 10 years old.
 o I need to continue support for a few old kernel versions,
   specifically linux-3.15 and linux-2.6.30.  That includes
   keeping them working with new toolchain versions.  I need
   to back port fixes to these old kernels.
 o When problems arise I sometimes need to use git bisect
   back to old kernel versions.  When I do the bisect I often
   have fixes and local debug patches that I apply to the
   bisected tree before building.
 o Source code changes between versions causes patch conflicts
   that need to be manually resolved.  This can be error prone
   and very time consuming on a long bisect session.

My decision to reject this patch and others like it is in
attempt to minimize the code maintenance effort.  If you have
patches that fix bugs, upgrade depreciated features, or
generally improve functionality please submit them for
review.

-Geoff

Re: ps3: Improve a size determination in five functions

[SF Markus Elfring]

> Some observations:
> 
>  o Your patch fixes no bug nor replaces any depreciated feature.

How do you think about information from the section “14) Allocating memory”
in the document “coding-style.rst” for the shown source code transformation?


>  o There will be no functional change; …

Yes. - The suggested adjustment should work in this way generally.

Regards,
Markus

Re: ps3: Improve a size determination in five functions

[SF Markus Elfring]

> Some observations:
> 
>  o Your patch fixes no bug nor replaces any depreciated feature.

How do you think about information from the section “14) Allocating memory”
in the document “coding-style.rst” for the shown source code transformation?


>  o There will be no functional change; …

Yes. - The suggested adjustment should work in this way generally.

Regards,
Markus

Re: ps3: Improve a size determination in five functions

[Geoff Levand]

On 12/20/2017 01:20 PM, SF Markus Elfring wrote:
>>  o Your patch fixes no bug nor replaces any depreciated feature.
> 
> How do you think about information from the section “14) Allocating memory”
> in the document “coding-style.rst” for the shown source code transformation?

In terms of importance, I would put maintenance and user support as more
important than coding style.

Regarding Section 14 of coding-style.rst specifically, as I mentioned the
PS3 support is over 10 years old.  I don't expect a change to the type of
any structures.  If there are type changes, then we can update the
allocation size parameters at that time.

-Geoff

Re: ps3: Improve a size determination in five functions

[Geoff Levand]

On 12/20/2017 01:20 PM, SF Markus Elfring wrote:
>>  o Your patch fixes no bug nor replaces any depreciated feature.
> 
> How do you think about information from the section “14) Allocating memory”
> in the document “coding-style.rst” for the shown source code transformation?

In terms of importance, I would put maintenance and user support as more
important than coding style.

Regarding Section 14 of coding-style.rst specifically, as I mentioned the
PS3 support is over 10 years old.  I don't expect a change to the type of
any structures.  If there are type changes, then we can update the
allocation size parameters at that time.

-Geoff

Re: [PATCH 0/2] PS3: Adjustments for six function implementations

[Geoff Levand]

Hi,

On 12/16/2017 05:50 AM, SF Markus Elfring wrote:
> Markus Elfring (2):
>   Delete an error message for a failed memory allocation in two functions
>   Improve a size determination in five functions

As I mentioned before, please keep your commit message subjects to
50 chars or less.

-Geoff

Re: [PATCH 0/2] PS3: Adjustments for six function implementations

[Geoff Levand]

Hi,

On 12/16/2017 05:50 AM, SF Markus Elfring wrote:
> Markus Elfring (2):
>   Delete an error message for a failed memory allocation in two functions
>   Improve a size determination in five functions

As I mentioned before, please keep your commit message subjects to
50 chars or less.

-Geoff