| Message ID | 7aed1208-aa55-edb6-54be-f47fb1498bfd@users.sourceforge.net (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | PS3: Adjustments for six function implementations | expand |
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
>> 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
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
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
> 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
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
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;