LinuxPPC-Dev Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/2] PS3: Adjustments for six function implementations
@ 2017-12-16 13:50 SF Markus Elfring
  2017-12-16 13:51 ` [PATCH 1/2] ps3: Delete an error message for a failed memory allocation in two functions SF Markus Elfring
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: SF Markus Elfring @ 2017-12-16 13:50 UTC (permalink / raw)
  To: linuxppc-dev, Benjamin Herrenschmidt, Geoff Levand,
	Michael Ellerman, Paul Mackerras
  Cc: LKML, kernel-janitors

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/2] ps3: Delete an error message for a failed memory allocation in two functions
  2017-12-16 13:50 [PATCH 0/2] PS3: Adjustments for six function implementations SF Markus Elfring
@ 2017-12-16 13:51 ` SF Markus Elfring
  2017-12-18 18:22   ` Geoff Levand
  2017-12-16 13:54 ` [PATCH 2/2] ps3: Improve a size determination in five functions SF Markus Elfring
  2017-12-18 18:32 ` [PATCH 0/2] PS3: Adjustments for six function implementations Geoff Levand
  2 siblings, 1 reply; 11+ messages in thread
From: SF Markus Elfring @ 2017-12-16 13:51 UTC (permalink / raw)
  To: linuxppc-dev, Benjamin Herrenschmidt, Geoff Levand,
	Michael Ellerman, Paul Mackerras
  Cc: LKML, kernel-janitors

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 2/2] ps3: Improve a size determination in five functions
  2017-12-16 13:50 [PATCH 0/2] PS3: Adjustments for six function implementations SF Markus Elfring
  2017-12-16 13:51 ` [PATCH 1/2] ps3: Delete an error message for a failed memory allocation in two functions SF Markus Elfring
@ 2017-12-16 13:54 ` SF Markus Elfring
  2017-12-18 18:22   ` Geoff Levand
  2017-12-20 19:58   ` [PATCH 2/2] " Geoff Levand
  2017-12-18 18:32 ` [PATCH 0/2] PS3: Adjustments for six function implementations Geoff Levand
  2 siblings, 2 replies; 11+ messages in thread
From: SF Markus Elfring @ 2017-12-16 13:54 UTC (permalink / raw)
  To: linuxppc-dev, Benjamin Herrenschmidt, Geoff Levand,
	Michael Ellerman, Paul Mackerras
  Cc: LKML, kernel-janitors

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] ps3: Delete an error message for a failed memory allocation in two functions
  2017-12-16 13:51 ` [PATCH 1/2] ps3: Delete an error message for a failed memory allocation in two functions SF Markus Elfring
@ 2017-12-18 18:22   ` Geoff Levand
  0 siblings, 0 replies; 11+ messages in thread
From: Geoff Levand @ 2017-12-18 18:22 UTC (permalink / raw)
  To: SF Markus Elfring, linuxppc-dev, Benjamin Herrenschmidt,
	Michael Ellerman, Paul Mackerras
  Cc: LKML, kernel-janitors

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] ps3: Improve a size determination in five functions
  2017-12-16 13:54 ` [PATCH 2/2] ps3: Improve a size determination in five functions SF Markus Elfring
@ 2017-12-18 18:22   ` Geoff Levand
  2017-12-18 20:45     ` SF Markus Elfring
  2017-12-20 19:58   ` [PATCH 2/2] " Geoff Levand
  1 sibling, 1 reply; 11+ messages in thread
From: Geoff Levand @ 2017-12-18 18:22 UTC (permalink / raw)
  To: SF Markus Elfring, linuxppc-dev, Benjamin Herrenschmidt,
	Michael Ellerman, Paul Mackerras
  Cc: LKML, kernel-janitors

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/2] PS3: Adjustments for six function implementations
  2017-12-16 13:50 [PATCH 0/2] PS3: Adjustments for six function implementations SF Markus Elfring
  2017-12-16 13:51 ` [PATCH 1/2] ps3: Delete an error message for a failed memory allocation in two functions SF Markus Elfring
  2017-12-16 13:54 ` [PATCH 2/2] ps3: Improve a size determination in five functions SF Markus Elfring
@ 2017-12-18 18:32 ` Geoff Levand
  2 siblings, 0 replies; 11+ messages in thread
From: Geoff Levand @ 2017-12-18 18:32 UTC (permalink / raw)
  To: SF Markus Elfring, linuxppc-dev, Benjamin Herrenschmidt,
	Michael Ellerman, Paul Mackerras
  Cc: LKML, kernel-janitors

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: ps3: Improve a size determination in five functions
  2017-12-18 18:22   ` Geoff Levand
@ 2017-12-18 20:45     ` SF Markus Elfring
  2017-12-18 20:52       ` Geoff Levand
  0 siblings, 1 reply; 11+ messages in thread
From: SF Markus Elfring @ 2017-12-18 20:45 UTC (permalink / raw)
  To: Geoff Levand, linuxppc-dev
  Cc: Benjamin Herrenschmidt, Michael Ellerman, Paul Mackerras, LKML,
	kernel-janitors

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: ps3: Improve a size determination in five functions
  2017-12-18 20:45     ` SF Markus Elfring
@ 2017-12-18 20:52       ` Geoff Levand
  0 siblings, 0 replies; 11+ messages in thread
From: Geoff Levand @ 2017-12-18 20:52 UTC (permalink / raw)
  To: SF Markus Elfring, linuxppc-dev
  Cc: Benjamin Herrenschmidt, Michael Ellerman, Paul Mackerras, LKML,
	kernel-janitors

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] ps3: Improve a size determination in five functions
  2017-12-16 13:54 ` [PATCH 2/2] ps3: Improve a size determination in five functions SF Markus Elfring
  2017-12-18 18:22   ` Geoff Levand
@ 2017-12-20 19:58   ` Geoff Levand
  2017-12-20 21:20     ` SF Markus Elfring
  1 sibling, 1 reply; 11+ messages in thread
From: Geoff Levand @ 2017-12-20 19:58 UTC (permalink / raw)
  To: SF Markus Elfring, linuxppc-dev, Benjamin Herrenschmidt,
	Michael Ellerman, Paul Mackerras
  Cc: LKML, kernel-janitors

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: ps3: Improve a size determination in five functions
  2017-12-20 19:58   ` [PATCH 2/2] " Geoff Levand
@ 2017-12-20 21:20     ` SF Markus Elfring
  2017-12-21  0:05       ` Geoff Levand
  0 siblings, 1 reply; 11+ messages in thread
From: SF Markus Elfring @ 2017-12-20 21:20 UTC (permalink / raw)
  To: Geoff Levand, linuxppc-dev
  Cc: Benjamin Herrenschmidt, Michael Ellerman, Paul Mackerras, LKML,
	kernel-janitors

> 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: ps3: Improve a size determination in five functions
  2017-12-20 21:20     ` SF Markus Elfring
@ 2017-12-21  0:05       ` Geoff Levand
  0 siblings, 0 replies; 11+ messages in thread
From: Geoff Levand @ 2017-12-21  0:05 UTC (permalink / raw)
  To: SF Markus Elfring, linuxppc-dev
  Cc: Benjamin Herrenschmidt, Michael Ellerman, Paul Mackerras, LKML,
	kernel-janitors

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, back to index

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-16 13:50 [PATCH 0/2] PS3: Adjustments for six function implementations SF Markus Elfring
2017-12-16 13:51 ` [PATCH 1/2] ps3: Delete an error message for a failed memory allocation in two functions SF Markus Elfring
2017-12-18 18:22   ` Geoff Levand
2017-12-16 13:54 ` [PATCH 2/2] ps3: Improve a size determination in five functions SF Markus Elfring
2017-12-18 18:22   ` Geoff Levand
2017-12-18 20:45     ` SF Markus Elfring
2017-12-18 20:52       ` Geoff Levand
2017-12-20 19:58   ` [PATCH 2/2] " Geoff Levand
2017-12-20 21:20     ` SF Markus Elfring
2017-12-21  0:05       ` Geoff Levand
2017-12-18 18:32 ` [PATCH 0/2] PS3: Adjustments for six function implementations Geoff Levand

LinuxPPC-Dev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linuxppc-dev/0 linuxppc-dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linuxppc-dev linuxppc-dev/ https://lore.kernel.org/linuxppc-dev \
		linuxppc-dev@lists.ozlabs.org linuxppc-dev@ozlabs.org
	public-inbox-index linuxppc-dev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.ozlabs.lists.linuxppc-dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git