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 
> 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 
> 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

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

[SF Markus Elfring]

From: Markus Elfring 
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 
---
 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, ,
-   sizeof(struct ps3_sys_manager_header));
+   int result = ps3_vuart_read(dev, , 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(>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