[PATCH v4] staging: qlge: emit debug and dump at same level

[PATCH v4] staging: qlge: emit debug and dump at same level

[Kaaira Gupta]

Simplify code in ql_mpi_core_to_log() by calling print_hex_dump()
instead of existing functions so that the debug and dump are
emitted at the same KERN_<LEVEL>

Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
---

changes since v1: make code of ql_mpi_core_to_log() simpler.
changes since v2: directly call the function instead of defining a
macro.
changes since v3: Remove prefix string.

----
---
 drivers/staging/qlge/qlge_dbg.c | 23 +++--------------------
 1 file changed, 3 insertions(+), 20 deletions(-)

diff --git a/drivers/staging/qlge/qlge_dbg.c b/drivers/staging/qlge/qlge_dbg.c
index c7af2548d119..0c5f6859e645 100644
--- a/drivers/staging/qlge/qlge_dbg.c
+++ b/drivers/staging/qlge/qlge_dbg.c
@@ -1324,27 +1324,10 @@ void ql_mpi_core_to_log(struct work_struct *work)
 {
 	struct ql_adapter *qdev =
 		container_of(work, struct ql_adapter, mpi_core_to_log.work);
-	u32 *tmp, count;
-	int i;
 
-	count = sizeof(struct ql_mpi_coredump) / sizeof(u32);
-	tmp = (u32 *)qdev->mpi_coredump;
-	netif_printk(qdev, drv, KERN_DEBUG, qdev->ndev,
-		     "Core is dumping to log file!\n");
-
-	for (i = 0; i < count; i += 8) {
-		pr_err("%.08x: %.08x %.08x %.08x %.08x %.08x "
-			"%.08x %.08x %.08x\n", i,
-			tmp[i + 0],
-			tmp[i + 1],
-			tmp[i + 2],
-			tmp[i + 3],
-			tmp[i + 4],
-			tmp[i + 5],
-			tmp[i + 6],
-			tmp[i + 7]);
-		msleep(5);
-	}
+	print_hex_dump(KERN_DEBUG, "", DUMP_PREFIX_OFFSET,
+		       32, 4, qdev->mpi_coredump,
+		       sizeof(*qdev->mpi_coredump), false);
 }
 
 #ifdef QL_REG_DUMP
-- 
2.17.1

Re: [PATCH v4] staging: qlge: emit debug and dump at same level

[Joe Perches]

On Mon, 2020-02-24 at 13:54 +0530, Kaaira Gupta wrote:
> Simplify code in ql_mpi_core_to_log() by calling print_hex_dump()
> instead of existing functions so that the debug and dump are
> emitted at the same KERN_<LEVEL>
[]
> diff --git a/drivers/staging/qlge/qlge_dbg.c b/drivers/staging/qlge/qlge_dbg.c
[]
> @@ -1324,27 +1324,10 @@ void ql_mpi_core_to_log(struct work_struct *work)
>  {
>  	struct ql_adapter *qdev =
>  		container_of(work, struct ql_adapter, mpi_core_to_log.work);
> -	u32 *tmp, count;
> -	int i;
>  
> -	count = sizeof(struct ql_mpi_coredump) / sizeof(u32);
> -	tmp = (u32 *)qdev->mpi_coredump;
> -	netif_printk(qdev, drv, KERN_DEBUG, qdev->ndev,
> -		     "Core is dumping to log file!\n");

There is no real need to delete this line.

And if you really want to, it'd be better to mention
the removal in the commit message description.

As is for this change, there is no "debug" and "dump"
as the commit message description shows, just "dump".

Re: [PATCH v4] staging: qlge: emit debug and dump at same level

[Kaaira Gupta]

On Mon, Feb 24, 2020 at 05:38:09AM -0800, Joe Perches wrote:
> On Mon, 2020-02-24 at 13:54 +0530, Kaaira Gupta wrote:
> > Simplify code in ql_mpi_core_to_log() by calling print_hex_dump()
> > instead of existing functions so that the debug and dump are
> > emitted at the same KERN_<LEVEL>
> []
> > diff --git a/drivers/staging/qlge/qlge_dbg.c b/drivers/staging/qlge/qlge_dbg.c
> []
> > @@ -1324,27 +1324,10 @@ void ql_mpi_core_to_log(struct work_struct *work)
> >  {
> >  	struct ql_adapter *qdev =
> >  		container_of(work, struct ql_adapter, mpi_core_to_log.work);
> > -	u32 *tmp, count;
> > -	int i;
> >  
> > -	count = sizeof(struct ql_mpi_coredump) / sizeof(u32);
> > -	tmp = (u32 *)qdev->mpi_coredump;
> > -	netif_printk(qdev, drv, KERN_DEBUG, qdev->ndev,
> > -		     "Core is dumping to log file!\n");
> 
> There is no real need to delete this line.
> 
> And if you really want to, it'd be better to mention
> the removal in the commit message description.
> 
> As is for this change, there is no "debug" and "dump"
> as the commit message description shows, just "dump".

This patch has already been added to the tree, if I amend the commit now
using git rebase, won't it affect the upstream as the SHA-1 of the
commit and it's children will change?

> 
> 
> 
> 

Re: [PATCH v4] staging: qlge: emit debug and dump at same level

[Joe Perches]

On Mon, 2020-02-24 at 22:17 +0530, Kaaira Gupta wrote:
> On Mon, Feb 24, 2020 at 05:38:09AM -0800, Joe Perches wrote:
> > On Mon, 2020-02-24 at 13:54 +0530, Kaaira Gupta wrote:
> > > Simplify code in ql_mpi_core_to_log() by calling print_hex_dump()
> > > instead of existing functions so that the debug and dump are
> > > emitted at the same KERN_<LEVEL>
> > []
> > > diff --git a/drivers/staging/qlge/qlge_dbg.c b/drivers/staging/qlge/qlge_dbg.c
> > []
> > > @@ -1324,27 +1324,10 @@ void ql_mpi_core_to_log(struct work_struct *work)
> > >  {
> > >  	struct ql_adapter *qdev =
> > >  		container_of(work, struct ql_adapter, mpi_core_to_log.work);
> > > -	u32 *tmp, count;
> > > -	int i;
> > >  
> > > -	count = sizeof(struct ql_mpi_coredump) / sizeof(u32);
> > > -	tmp = (u32 *)qdev->mpi_coredump;
> > > -	netif_printk(qdev, drv, KERN_DEBUG, qdev->ndev,
> > > -		     "Core is dumping to log file!\n");
> > 
> > There is no real need to delete this line.
> > 
> > And if you really want to, it'd be better to mention
> > the removal in the commit message description.
> > 
> > As is for this change, there is no "debug" and "dump"
> > as the commit message description shows, just "dump".
> 
> This patch has already been added to the tree,

What tree is that?
It's not in -next as of right now.

>  if I amend the commit now
> using git rebase, won't it affect the upstream as the SHA-1 of the
> commit and it's children will change?

You are sending patches not pull requests.

If it's really in an actual tree that people
care about, send another patch putting the
netif_printk back.

Re: [PATCH v4] staging: qlge: emit debug and dump at same level

[Kaaira Gupta]

On Mon, Feb 24, 2020 at 08:54:43AM -0800, Joe Perches wrote:
> On Mon, 2020-02-24 at 22:17 +0530, Kaaira Gupta wrote:
> > On Mon, Feb 24, 2020 at 05:38:09AM -0800, Joe Perches wrote:
> > > On Mon, 2020-02-24 at 13:54 +0530, Kaaira Gupta wrote:
> > > > Simplify code in ql_mpi_core_to_log() by calling print_hex_dump()
> > > > instead of existing functions so that the debug and dump are
> > > > emitted at the same KERN_<LEVEL>
> > > []
> > > > diff --git a/drivers/staging/qlge/qlge_dbg.c b/drivers/staging/qlge/qlge_dbg.c
> > > []
> > > > @@ -1324,27 +1324,10 @@ void ql_mpi_core_to_log(struct work_struct *work)
> > > >  {
> > > >  	struct ql_adapter *qdev =
> > > >  		container_of(work, struct ql_adapter, mpi_core_to_log.work);
> > > > -	u32 *tmp, count;
> > > > -	int i;
> > > >  
> > > > -	count = sizeof(struct ql_mpi_coredump) / sizeof(u32);
> > > > -	tmp = (u32 *)qdev->mpi_coredump;
> > > > -	netif_printk(qdev, drv, KERN_DEBUG, qdev->ndev,
> > > > -		     "Core is dumping to log file!\n");
> > > 
> > > There is no real need to delete this line.
> > > 
> > > And if you really want to, it'd be better to mention
> > > the removal in the commit message description.
> > > 
> > > As is for this change, there is no "debug" and "dump"
> > > as the commit message description shows, just "dump".
> > 
> > This patch has already been added to the tree,
> 
> What tree is that?
> It's not in -next as of right now.

Its in staging-next right now.
This is the link: https://lore.kernel.org/driverdev-devel/cba75ee4d88afdf118631510ad0f971e42c1a31c.camel@perches.com/

> 
> >  if I amend the commit now
> > using git rebase, won't it affect the upstream as the SHA-1 of the
> > commit and it's children will change?
> 
> You are sending patches not pull requests.
> 
> If it's really in an actual tree that people
> care about, send another patch putting the
> netif_printk back.

I'll submit a patch, but can you please explain me why this function is
still needed when we are already using print_hex_dump()?

> 
> 

Re: [PATCH v4] staging: qlge: emit debug and dump at same level

[Joe Perches]

On Mon, 2020-02-24 at 22:48 +0530, Kaaira Gupta wrote:
> On Mon, Feb 24, 2020 at 08:54:43AM -0800, Joe Perches wrote:
> > On Mon, 2020-02-24 at 22:17 +0530, Kaaira Gupta wrote:
> > > On Mon, Feb 24, 2020 at 05:38:09AM -0800, Joe Perches wrote:
> > > > On Mon, 2020-02-24 at 13:54 +0530, Kaaira Gupta wrote:
> > > > > Simplify code in ql_mpi_core_to_log() by calling print_hex_dump()
> > > > > instead of existing functions so that the debug and dump are
> > > > > emitted at the same KERN_<LEVEL>
> > > > []
> > > > > diff --git a/drivers/staging/qlge/qlge_dbg.c b/drivers/staging/qlge/qlge_dbg.c
> > > > []
> > > > > @@ -1324,27 +1324,10 @@ void ql_mpi_core_to_log(struct work_struct *work)
> > > > >  {
> > > > >  	struct ql_adapter *qdev =
> > > > >  		container_of(work, struct ql_adapter, mpi_core_to_log.work);
> > > > > -	u32 *tmp, count;
> > > > > -	int i;
> > > > >  
> > > > > -	count = sizeof(struct ql_mpi_coredump) / sizeof(u32);
> > > > > -	tmp = (u32 *)qdev->mpi_coredump;
> > > > > -	netif_printk(qdev, drv, KERN_DEBUG, qdev->ndev,
> > > > > -		     "Core is dumping to log file!\n");
> > > > 
> > > > There is no real need to delete this line.
> > > > 
> > > > And if you really want to, it'd be better to mention
> > > > the removal in the commit message description.
> > > > 
> > > > As is for this change, there is no "debug" and "dump"
> > > > as the commit message description shows, just "dump".
> > > 
> > > This patch has already been added to the tree,
> > 
> > What tree is that?
> > It's not in -next as of right now.
> 
> Its in staging-next right now.
> This is the link: https://lore.kernel.org/driverdev-devel/cba75ee4d88afdf118631510ad0f971e42c1a31c.camel@perches.com/
> 
> > >  if I amend the commit now
> > > using git rebase, won't it affect the upstream as the SHA-1 of the
> > > commit and it's children will change?
> > 
> > You are sending patches not pull requests.
> > 
> > If it's really in an actual tree that people
> > care about, send another patch putting the
> > netif_printk back.
> 
> I'll submit a patch, but can you please explain me why this function is
> still needed when we are already using print_hex_dump()?

Your commit message did not match the code.

You are changing the code and the output without explanation.

It's fine to both change the code and the output when appropriate.
it's not fine to do so by misstating what you are doing.

Re: [PATCH v4] staging: qlge: emit debug and dump at same level

[Greg Kroah-Hartman]

On Mon, Feb 24, 2020 at 01:54:48PM +0530, Kaaira Gupta wrote:
> Simplify code in ql_mpi_core_to_log() by calling print_hex_dump()
> instead of existing functions so that the debug and dump are
> emitted at the same KERN_<LEVEL>
> 
> Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
> ---
> 
> changes since v1: make code of ql_mpi_core_to_log() simpler.
> changes since v2: directly call the function instead of defining a
> macro.
> changes since v3: Remove prefix string.
> 
> ----
> ---
>  drivers/staging/qlge/qlge_dbg.c | 23 +++--------------------
>  1 file changed, 3 insertions(+), 20 deletions(-)

Does not apply to my tree :(