[PATCH 0/4] char-TPM: Adjustments for ten function implementations

[PATCH 0/4] char-TPM: Adjustments for ten function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 16 Oct 2017 19:12:34 +0200

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

Markus Elfring (4):
  Delete an error message for a failed memory allocation
    in tpm_ascii_bios_measurements_show()
  Delete an error message for a failed memory allocation in tpm_ibmvtpm_probe()
  Improve a size determination in nine functions
  Less checks in tpm_ibmvtpm_probe() after error detection

 drivers/char/tpm/st33zp24/i2c.c      |  3 +--
 drivers/char/tpm/st33zp24/spi.c      |  3 +--
 drivers/char/tpm/st33zp24/st33zp24.c |  3 +--
 drivers/char/tpm/tpm1_eventlog.c     |  5 +----
 drivers/char/tpm/tpm_crb.c           |  2 +-
 drivers/char/tpm/tpm_i2c_atmel.c     |  2 +-
 drivers/char/tpm/tpm_i2c_nuvoton.c   |  2 +-
 drivers/char/tpm/tpm_ibmvtpm.c       | 23 +++++++++--------------
 drivers/char/tpm/tpm_tis.c           |  2 +-
 drivers/char/tpm/tpm_tis_spi.c       |  3 +--
 10 files changed, 18 insertions(+), 30 deletions(-)

-- 
2.14.2

[PATCH 1/4] char/tpm: Delete an error message for a failed memory allocation in tpm_ascii_bios_measurements_show()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 16 Oct 2017 17:43:55 +0200

Omit an extra message for a memory allocation failure in this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/char/tpm/tpm1_eventlog.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/char/tpm/tpm1_eventlog.c b/drivers/char/tpm/tpm1_eventlog.c
index 9a8605e500b5..9749469cb823 100644
--- a/drivers/char/tpm/tpm1_eventlog.c
+++ b/drivers/char/tpm/tpm1_eventlog.c
@@ -280,11 +280,8 @@ static int tpm_ascii_bios_measurements_show(struct seq_file *m, void *v)
 	    (unsigned char *)(v + sizeof(struct tcpa_event));
 
 	eventname = kmalloc(MAX_TEXT_EVENT, GFP_KERNEL);
-	if (!eventname) {
-		printk(KERN_ERR "%s: ERROR - No Memory for event name\n ",
-		       __func__);
+	if (!eventname)
 		return -EFAULT;
-	}
 
 	/* 1st: PCR */
 	seq_printf(m, "%2d ", do_endian_conversion(event->pcr_index));
-- 
2.14.2

[PATCH 2/4] char/tpm: Delete an error message for a failed memory allocation in tpm_ibmvtpm_probe()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 16 Oct 2017 18:08:23 +0200

Omit an extra message for a memory allocation failure in this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/char/tpm/tpm_ibmvtpm.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/char/tpm/tpm_ibmvtpm.c b/drivers/char/tpm/tpm_ibmvtpm.c
index 25f6e2665385..b18148ef2612 100644
--- a/drivers/char/tpm/tpm_ibmvtpm.c
+++ b/drivers/char/tpm/tpm_ibmvtpm.c
@@ -609,10 +609,8 @@ static int tpm_ibmvtpm_probe(struct vio_dev *vio_dev,
 		return PTR_ERR(chip);
 
 	ibmvtpm = kzalloc(sizeof(struct ibmvtpm_dev), GFP_KERNEL);
-	if (!ibmvtpm) {
-		dev_err(dev, "kzalloc for ibmvtpm failed\n");
+	if (!ibmvtpm)
 		goto cleanup;
-	}
 
 	ibmvtpm->dev = dev;
 	ibmvtpm->vdev = vio_dev;
-- 
2.14.2

[PATCH 3/4] char/tpm: Improve a size determination in nine functions

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 16 Oct 2017 18:28:17 +0200

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

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/char/tpm/st33zp24/i2c.c      | 3 +--
 drivers/char/tpm/st33zp24/spi.c      | 3 +--
 drivers/char/tpm/st33zp24/st33zp24.c | 3 +--
 drivers/char/tpm/tpm_crb.c           | 2 +-
 drivers/char/tpm/tpm_i2c_atmel.c     | 2 +-
 drivers/char/tpm/tpm_i2c_nuvoton.c   | 2 +-
 drivers/char/tpm/tpm_ibmvtpm.c       | 2 +-
 drivers/char/tpm/tpm_tis.c           | 2 +-
 drivers/char/tpm/tpm_tis_spi.c       | 3 +--
 9 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/char/tpm/st33zp24/i2c.c b/drivers/char/tpm/st33zp24/i2c.c
index be5d1abd3e8e..d0cb25688485 100644
--- a/drivers/char/tpm/st33zp24/i2c.c
+++ b/drivers/char/tpm/st33zp24/i2c.c
@@ -245,8 +245,7 @@ static int st33zp24_i2c_probe(struct i2c_client *client,
 		return -ENODEV;
 	}
 
-	phy = devm_kzalloc(&client->dev, sizeof(struct st33zp24_i2c_phy),
-			   GFP_KERNEL);
+	phy = devm_kzalloc(&client->dev, sizeof(*phy), GFP_KERNEL);
 	if (!phy)
 		return -ENOMEM;
 
diff --git a/drivers/char/tpm/st33zp24/spi.c b/drivers/char/tpm/st33zp24/spi.c
index 0fc4f20b5f83..c952df9244c8 100644
--- a/drivers/char/tpm/st33zp24/spi.c
+++ b/drivers/char/tpm/st33zp24/spi.c
@@ -358,8 +358,7 @@ static int st33zp24_spi_probe(struct spi_device *dev)
 		return -ENODEV;
 	}
 
-	phy = devm_kzalloc(&dev->dev, sizeof(struct st33zp24_spi_phy),
-			   GFP_KERNEL);
+	phy = devm_kzalloc(&dev->dev, sizeof(*phy), GFP_KERNEL);
 	if (!phy)
 		return -ENOMEM;
 
diff --git a/drivers/char/tpm/st33zp24/st33zp24.c b/drivers/char/tpm/st33zp24/st33zp24.c
index 4d1dc8b46877..0686a129268c 100644
--- a/drivers/char/tpm/st33zp24/st33zp24.c
+++ b/drivers/char/tpm/st33zp24/st33zp24.c
@@ -533,8 +533,7 @@ int st33zp24_probe(void *phy_id, const struct st33zp24_phy_ops *ops,
 	if (IS_ERR(chip))
 		return PTR_ERR(chip);
 
-	tpm_dev = devm_kzalloc(dev, sizeof(struct st33zp24_dev),
-			       GFP_KERNEL);
+	tpm_dev = devm_kzalloc(dev, sizeof(*tpm_dev), GFP_KERNEL);
 	if (!tpm_dev)
 		return -ENOMEM;
 
diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 7b3c2a8aa9de..343c46e8560f 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -557,7 +557,7 @@ static int crb_acpi_add(struct acpi_device *device)
 	if (sm == ACPI_TPM2_MEMORY_MAPPED)
 		return -ENODEV;
 
-	priv = devm_kzalloc(dev, sizeof(struct crb_priv), GFP_KERNEL);
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
 
diff --git a/drivers/char/tpm/tpm_i2c_atmel.c b/drivers/char/tpm/tpm_i2c_atmel.c
index 95ce2e9ccdc6..2d0df930a76d 100644
--- a/drivers/char/tpm/tpm_i2c_atmel.c
+++ b/drivers/char/tpm/tpm_i2c_atmel.c
@@ -165,7 +165,7 @@ static int i2c_atmel_probe(struct i2c_client *client,
 	if (IS_ERR(chip))
 		return PTR_ERR(chip);
 
-	priv = devm_kzalloc(dev, sizeof(struct priv_data), GFP_KERNEL);
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
 
diff --git a/drivers/char/tpm/tpm_i2c_nuvoton.c b/drivers/char/tpm/tpm_i2c_nuvoton.c
index c6428771841f..5983d52eb6d9 100644
--- a/drivers/char/tpm/tpm_i2c_nuvoton.c
+++ b/drivers/char/tpm/tpm_i2c_nuvoton.c
@@ -551,7 +551,7 @@ static int i2c_nuvoton_probe(struct i2c_client *client,
 	if (IS_ERR(chip))
 		return PTR_ERR(chip);
 
-	priv = devm_kzalloc(dev, sizeof(struct priv_data), GFP_KERNEL);
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
 
diff --git a/drivers/char/tpm/tpm_ibmvtpm.c b/drivers/char/tpm/tpm_ibmvtpm.c
index b18148ef2612..a4b462a77b99 100644
--- a/drivers/char/tpm/tpm_ibmvtpm.c
+++ b/drivers/char/tpm/tpm_ibmvtpm.c
@@ -608,7 +608,7 @@ static int tpm_ibmvtpm_probe(struct vio_dev *vio_dev,
 	if (IS_ERR(chip))
 		return PTR_ERR(chip);
 
-	ibmvtpm = kzalloc(sizeof(struct ibmvtpm_dev), GFP_KERNEL);
+	ibmvtpm = kzalloc(sizeof(*ibmvtpm), GFP_KERNEL);
 	if (!ibmvtpm)
 		goto cleanup;
 
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index ebd0e75a3e4d..0a3af60bab2a 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -294,7 +294,7 @@ static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info)
 	if (rc)
 		return rc;
 
-	phy = devm_kzalloc(dev, sizeof(struct tpm_tis_tcg_phy), GFP_KERNEL);
+	phy = devm_kzalloc(dev, sizeof(*phy), GFP_KERNEL);
 	if (phy == NULL)
 		return -ENOMEM;
 
diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
index 424ff2fde1f2..7cabb12d0b3a 100644
--- a/drivers/char/tpm/tpm_tis_spi.c
+++ b/drivers/char/tpm/tpm_tis_spi.c
@@ -200,8 +200,7 @@ static int tpm_tis_spi_probe(struct spi_device *dev)
 {
 	struct tpm_tis_spi_phy *phy;
 
-	phy = devm_kzalloc(&dev->dev, sizeof(struct tpm_tis_spi_phy),
-			   GFP_KERNEL);
+	phy = devm_kzalloc(&dev->dev, sizeof(*phy), GFP_KERNEL);
 	if (!phy)
 		return -ENOMEM;
 
-- 
2.14.2

[PATCH 4/4] char/tpm: Less checks in tpm_ibmvtpm_probe() after error detection

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 16 Oct 2017 19:00:34 +0200

Two pointer checks could be repeated by the tpm_ibmvtpm_probe() function
during error handling even if the relevant properties can be determined
for the involved variables before by source code analysis.

* Return directly after a call of the function "kzalloc" failed
  at the beginning.

* Adjust jump targets so that extra checks can be omitted at the end.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/char/tpm/tpm_ibmvtpm.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/char/tpm/tpm_ibmvtpm.c b/drivers/char/tpm/tpm_ibmvtpm.c
index a4b462a77b99..b8dda7546f64 100644
--- a/drivers/char/tpm/tpm_ibmvtpm.c
+++ b/drivers/char/tpm/tpm_ibmvtpm.c
@@ -610,7 +610,7 @@ static int tpm_ibmvtpm_probe(struct vio_dev *vio_dev,
 
 	ibmvtpm = kzalloc(sizeof(*ibmvtpm), GFP_KERNEL);
 	if (!ibmvtpm)
-		goto cleanup;
+		return -ENOMEM;
 
 	ibmvtpm->dev = dev;
 	ibmvtpm->vdev = vio_dev;
@@ -619,7 +619,7 @@ static int tpm_ibmvtpm_probe(struct vio_dev *vio_dev,
 	crq_q->crq_addr = (struct ibmvtpm_crq *)get_zeroed_page(GFP_KERNEL);
 	if (!crq_q->crq_addr) {
 		dev_err(dev, "Unable to allocate memory for crq_addr\n");
-		goto cleanup;
+		goto free_tpm;
 	}
 
 	crq_q->num_entry = CRQ_RES_BUF_SIZE / sizeof(*crq_q->crq_addr);
@@ -629,7 +629,7 @@ static int tpm_ibmvtpm_probe(struct vio_dev *vio_dev,
 
 	if (dma_mapping_error(dev, ibmvtpm->crq_dma_handle)) {
 		dev_err(dev, "dma mapping failed\n");
-		goto cleanup;
+		goto free_page;
 	}
 
 	rc = plpar_hcall_norets(H_REG_CRQ, vio_dev->unit_address,
@@ -683,13 +683,10 @@ static int tpm_ibmvtpm_probe(struct vio_dev *vio_dev,
 reg_crq_cleanup:
 	dma_unmap_single(dev, ibmvtpm->crq_dma_handle, CRQ_RES_BUF_SIZE,
 			 DMA_BIDIRECTIONAL);
-cleanup:
-	if (ibmvtpm) {
-		if (crq_q->crq_addr)
-			free_page((unsigned long)crq_q->crq_addr);
-		kfree(ibmvtpm);
-	}
-
+free_page:
+	free_page((unsigned long)crq_q->crq_addr);
+free_tpm:
+	kfree(ibmvtpm);
 	return rc;
 }
 
-- 
2.14.2

Re: [PATCH 0/4] char-TPM: Adjustments for ten function implementations

[Jarkko Sakkinen]

On Mon, Oct 16, 2017 at 07:30:13PM +0200, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 16 Oct 2017 19:12:34 +0200
> 
> A few update suggestions were taken into account
> from static source code analysis.
> 
> Markus Elfring (4):
>   Delete an error message for a failed memory allocation
>     in tpm_ascii_bios_measurements_show()
>   Delete an error message for a failed memory allocation in tpm_ibmvtpm_probe()
>   Improve a size determination in nine functions
>   Less checks in tpm_ibmvtpm_probe() after error detection
> 
>  drivers/char/tpm/st33zp24/i2c.c      |  3 +--
>  drivers/char/tpm/st33zp24/spi.c      |  3 +--
>  drivers/char/tpm/st33zp24/st33zp24.c |  3 +--
>  drivers/char/tpm/tpm1_eventlog.c     |  5 +----
>  drivers/char/tpm/tpm_crb.c           |  2 +-
>  drivers/char/tpm/tpm_i2c_atmel.c     |  2 +-
>  drivers/char/tpm/tpm_i2c_nuvoton.c   |  2 +-
>  drivers/char/tpm/tpm_ibmvtpm.c       | 23 +++++++++--------------
>  drivers/char/tpm/tpm_tis.c           |  2 +-
>  drivers/char/tpm/tpm_tis_spi.c       |  3 +--
>  10 files changed, 18 insertions(+), 30 deletions(-)
> 
> -- 
> 2.14.2
> 

For some sparse errors I fixed a while ago I got review feedback that
one should explain what is wrong what the fix does and not tell tool
reported. And it really does make sense to me.

Describing the tool that was used to find the issues fits to the cover
letter but not to the commits themselves.

I think I recently accepted a small fix with a "tool generated commit
message" but I don't want to take it as a practice It was a minor
mistake from my side to accept such patch.

/Jarkko

Re: [PATCH 0/4] char-TPM: Adjustments for ten function implementations

[Jarkko Sakkinen]

On Mon, Oct 16, 2017 at 09:31:39PM +0300, Jarkko Sakkinen wrote:
> On Mon, Oct 16, 2017 at 07:30:13PM +0200, SF Markus Elfring wrote:
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > Date: Mon, 16 Oct 2017 19:12:34 +0200
> > 
> > A few update suggestions were taken into account
> > from static source code analysis.
> > 
> > Markus Elfring (4):
> >   Delete an error message for a failed memory allocation
> >     in tpm_ascii_bios_measurements_show()
> >   Delete an error message for a failed memory allocation in tpm_ibmvtpm_probe()
> >   Improve a size determination in nine functions
> >   Less checks in tpm_ibmvtpm_probe() after error detection
> > 
> >  drivers/char/tpm/st33zp24/i2c.c      |  3 +--
> >  drivers/char/tpm/st33zp24/spi.c      |  3 +--
> >  drivers/char/tpm/st33zp24/st33zp24.c |  3 +--
> >  drivers/char/tpm/tpm1_eventlog.c     |  5 +----
> >  drivers/char/tpm/tpm_crb.c           |  2 +-
> >  drivers/char/tpm/tpm_i2c_atmel.c     |  2 +-
> >  drivers/char/tpm/tpm_i2c_nuvoton.c   |  2 +-
> >  drivers/char/tpm/tpm_ibmvtpm.c       | 23 +++++++++--------------
> >  drivers/char/tpm/tpm_tis.c           |  2 +-
> >  drivers/char/tpm/tpm_tis_spi.c       |  3 +--
> >  10 files changed, 18 insertions(+), 30 deletions(-)
> > 
> > -- 
> > 2.14.2
> > 
> 
> For some sparse errors I fixed a while ago I got review feedback that
> one should explain what is wrong what the fix does and not tell tool
> reported. And it really does make sense to me.
> 
> Describing the tool that was used to find the issues fits to the cover
> letter but not to the commits themselves.
> 
> I think I recently accepted a small fix with a "tool generated commit
> message" but I don't want to take it as a practice It was a minor
> mistake from my side to accept such patch.

A minor complaint: all commits are missing "Fixes:" tag.

/Jarkko

Re: [PATCH 0/4] char-TPM: Adjustments for ten function implementations

[SF Markus Elfring]

> A minor complaint: all commits are missing "Fixes:" tag.

* Do you require it to be added to the commit messages?

* Would you like to get a finer patch granularity then?

* Do you find any more information missing?

Regards,
Markus

Re: [PATCH 0/4] char-TPM: Adjustments for ten function implementations

[Joe Perches]

On Mon, 2017-10-16 at 21:35 +0300, Jarkko Sakkinen wrote:
> A minor complaint: all commits are missing "Fixes:" tag.

None of these patches fix anything.
All are trivial changes without much of any impact.

Re: [PATCH 0/4] char-TPM: Adjustments for ten function implementations

[SF Markus Elfring]

>> A minor complaint: all commits are missing "Fixes:" tag.
> 
> None of these patches fix anything.

It depends on the view which you prefer.


> All are trivial changes without much of any impact.

I find that they improve the affected software another bit.
Other adjustments can be more noticeable.

Regards,
Markus

Re: [PATCH 0/4] char-TPM: Adjustments for ten function implementations

[Dan Carpenter]

On Mon, Oct 16, 2017 at 09:35:12PM +0300, Jarkko Sakkinen wrote:
> 
> A minor complaint: all commits are missing "Fixes:" tag.
> 

Fixes is only for bug fixes.  These don't fix any bugs.

regards,
dan carpenter

Re: [PATCH 0/4] char-TPM: Adjustments for ten function implementations

[Julia Lawall]

On Tue, 17 Oct 2017, Dan Carpenter wrote:

> On Mon, Oct 16, 2017 at 09:35:12PM +0300, Jarkko Sakkinen wrote:
> >
> > A minor complaint: all commits are missing "Fixes:" tag.
> >
>
> Fixes is only for bug fixes.  These don't fix any bugs.

0-day seems to put Fixes for everything.  Should they be removed when the
old code is undesirable but doesn't actually cause a crash, eg out of date
API.

julia


>
> regards,
> dan carpenter
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 0/4] char-TPM: Adjustments for ten function implementations

[SF Markus Elfring]

> Fixes is only for bug fixes.  These don't fix any bugs.

How do you distinguish these in questionable source code
from other error categories or software weaknesses?

Regards,
Markus

Re: [PATCH 0/4] char-TPM: Adjustments for ten function implementations

[Dan Carpenter]

On Tue, Oct 17, 2017 at 10:56:42AM +0200, Julia Lawall wrote:
> 
> 
> On Tue, 17 Oct 2017, Dan Carpenter wrote:
> 
> > On Mon, Oct 16, 2017 at 09:35:12PM +0300, Jarkko Sakkinen wrote:
> > >
> > > A minor complaint: all commits are missing "Fixes:" tag.
> > >
> >
> > Fixes is only for bug fixes.  These don't fix any bugs.
> 
> 0-day seems to put Fixes for everything.  Should they be removed when the
> old code is undesirable but doesn't actually cause a crash, eg out of date
> API.

Yeah, I feel like Fixes tags don't belong for API updates and cleanups.

regards,
dan carpenter

Re: [PATCH 0/4] char-TPM: Adjustments for ten function implementations

[Julia Lawall]

On Tue, 17 Oct 2017, Dan Carpenter wrote:

> On Tue, Oct 17, 2017 at 10:56:42AM +0200, Julia Lawall wrote:
> >
> >
> > On Tue, 17 Oct 2017, Dan Carpenter wrote:
> >
> > > On Mon, Oct 16, 2017 at 09:35:12PM +0300, Jarkko Sakkinen wrote:
> > > >
> > > > A minor complaint: all commits are missing "Fixes:" tag.
> > > >
> > >
> > > Fixes is only for bug fixes.  These don't fix any bugs.
> >
> > 0-day seems to put Fixes for everything.  Should they be removed when the
> > old code is undesirable but doesn't actually cause a crash, eg out of date
> > API.
>
> Yeah, I feel like Fixes tags don't belong for API updates and cleanups.

OK, I will remove them from the patches that go through me where they
don't seem appropriate.

thanks,
julia

Re: [PATCH 3/4] char/tpm: Improve a size determination in nine functions

[Andy Shevchenko]

On Mon, 2017-10-16 at 19:33 +0200, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 16 Oct 2017 18:28:17 +0200
> 
> Replace the specification of data structures by pointer dereferences
> as the parameter for the operator "sizeof" to make the corresponding
> size
> determination a bit safer according to the Linux coding style
> convention.


This patch does one style in favor of the other.

At the end it's Jarkko's call, though I would NAK this as I think some
one already told this to you for some other similar patch(es).


I even would suggest to stop doing this noisy stuff, which keeps people
busy for nothing.

> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/char/tpm/st33zp24/i2c.c      | 3 +--
>  drivers/char/tpm/st33zp24/spi.c      | 3 +--
>  drivers/char/tpm/st33zp24/st33zp24.c | 3 +--
>  drivers/char/tpm/tpm_crb.c           | 2 +-
>  drivers/char/tpm/tpm_i2c_atmel.c     | 2 +-
>  drivers/char/tpm/tpm_i2c_nuvoton.c   | 2 +-
>  drivers/char/tpm/tpm_ibmvtpm.c       | 2 +-
>  drivers/char/tpm/tpm_tis.c           | 2 +-
>  drivers/char/tpm/tpm_tis_spi.c       | 3 +--
>  9 files changed, 9 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/char/tpm/st33zp24/i2c.c
> b/drivers/char/tpm/st33zp24/i2c.c
> index be5d1abd3e8e..d0cb25688485 100644
> --- a/drivers/char/tpm/st33zp24/i2c.c
> +++ b/drivers/char/tpm/st33zp24/i2c.c
> @@ -245,8 +245,7 @@ static int st33zp24_i2c_probe(struct i2c_client
> *client,
>  		return -ENODEV;
>  	}
>  
> -	phy = devm_kzalloc(&client->dev, sizeof(struct
> st33zp24_i2c_phy),
> -			   GFP_KERNEL);
> +	phy = devm_kzalloc(&client->dev, sizeof(*phy), GFP_KERNEL);
>  	if (!phy)
>  		return -ENOMEM;
>  
> diff --git a/drivers/char/tpm/st33zp24/spi.c
> b/drivers/char/tpm/st33zp24/spi.c
> index 0fc4f20b5f83..c952df9244c8 100644
> --- a/drivers/char/tpm/st33zp24/spi.c
> +++ b/drivers/char/tpm/st33zp24/spi.c
> @@ -358,8 +358,7 @@ static int st33zp24_spi_probe(struct spi_device
> *dev)
>  		return -ENODEV;
>  	}
>  
> -	phy = devm_kzalloc(&dev->dev, sizeof(struct
> st33zp24_spi_phy),
> -			   GFP_KERNEL);
> +	phy = devm_kzalloc(&dev->dev, sizeof(*phy), GFP_KERNEL);
>  	if (!phy)
>  		return -ENOMEM;
>  
> diff --git a/drivers/char/tpm/st33zp24/st33zp24.c
> b/drivers/char/tpm/st33zp24/st33zp24.c
> index 4d1dc8b46877..0686a129268c 100644
> --- a/drivers/char/tpm/st33zp24/st33zp24.c
> +++ b/drivers/char/tpm/st33zp24/st33zp24.c
> @@ -533,8 +533,7 @@ int st33zp24_probe(void *phy_id, const struct
> st33zp24_phy_ops *ops,
>  	if (IS_ERR(chip))
>  		return PTR_ERR(chip);
>  
> -	tpm_dev = devm_kzalloc(dev, sizeof(struct st33zp24_dev),
> -			       GFP_KERNEL);
> +	tpm_dev = devm_kzalloc(dev, sizeof(*tpm_dev), GFP_KERNEL);
>  	if (!tpm_dev)
>  		return -ENOMEM;
>  
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index 7b3c2a8aa9de..343c46e8560f 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -557,7 +557,7 @@ static int crb_acpi_add(struct acpi_device
> *device)
>  	if (sm == ACPI_TPM2_MEMORY_MAPPED)
>  		return -ENODEV;
>  
> -	priv = devm_kzalloc(dev, sizeof(struct crb_priv),
> GFP_KERNEL);
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>  	if (!priv)
>  		return -ENOMEM;
>  
> diff --git a/drivers/char/tpm/tpm_i2c_atmel.c
> b/drivers/char/tpm/tpm_i2c_atmel.c
> index 95ce2e9ccdc6..2d0df930a76d 100644
> --- a/drivers/char/tpm/tpm_i2c_atmel.c
> +++ b/drivers/char/tpm/tpm_i2c_atmel.c
> @@ -165,7 +165,7 @@ static int i2c_atmel_probe(struct i2c_client
> *client,
>  	if (IS_ERR(chip))
>  		return PTR_ERR(chip);
>  
> -	priv = devm_kzalloc(dev, sizeof(struct priv_data),
> GFP_KERNEL);
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>  	if (!priv)
>  		return -ENOMEM;
>  
> diff --git a/drivers/char/tpm/tpm_i2c_nuvoton.c
> b/drivers/char/tpm/tpm_i2c_nuvoton.c
> index c6428771841f..5983d52eb6d9 100644
> --- a/drivers/char/tpm/tpm_i2c_nuvoton.c
> +++ b/drivers/char/tpm/tpm_i2c_nuvoton.c
> @@ -551,7 +551,7 @@ static int i2c_nuvoton_probe(struct i2c_client
> *client,
>  	if (IS_ERR(chip))
>  		return PTR_ERR(chip);
>  
> -	priv = devm_kzalloc(dev, sizeof(struct priv_data),
> GFP_KERNEL);
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>  	if (!priv)
>  		return -ENOMEM;
>  
> diff --git a/drivers/char/tpm/tpm_ibmvtpm.c
> b/drivers/char/tpm/tpm_ibmvtpm.c
> index b18148ef2612..a4b462a77b99 100644
> --- a/drivers/char/tpm/tpm_ibmvtpm.c
> +++ b/drivers/char/tpm/tpm_ibmvtpm.c
> @@ -608,7 +608,7 @@ static int tpm_ibmvtpm_probe(struct vio_dev
> *vio_dev,
>  	if (IS_ERR(chip))
>  		return PTR_ERR(chip);
>  
> -	ibmvtpm = kzalloc(sizeof(struct ibmvtpm_dev), GFP_KERNEL);
> +	ibmvtpm = kzalloc(sizeof(*ibmvtpm), GFP_KERNEL);
>  	if (!ibmvtpm)
>  		goto cleanup;
>  
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index ebd0e75a3e4d..0a3af60bab2a 100644
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -294,7 +294,7 @@ static int tpm_tis_init(struct device *dev, struct
> tpm_info *tpm_info)
>  	if (rc)
>  		return rc;
>  
> -	phy = devm_kzalloc(dev, sizeof(struct tpm_tis_tcg_phy),
> GFP_KERNEL);
> +	phy = devm_kzalloc(dev, sizeof(*phy), GFP_KERNEL);
>  	if (phy == NULL)
>  		return -ENOMEM;
>  
> diff --git a/drivers/char/tpm/tpm_tis_spi.c
> b/drivers/char/tpm/tpm_tis_spi.c
> index 424ff2fde1f2..7cabb12d0b3a 100644
> --- a/drivers/char/tpm/tpm_tis_spi.c
> +++ b/drivers/char/tpm/tpm_tis_spi.c
> @@ -200,8 +200,7 @@ static int tpm_tis_spi_probe(struct spi_device
> *dev)
>  {
>  	struct tpm_tis_spi_phy *phy;
>  
> -	phy = devm_kzalloc(&dev->dev, sizeof(struct tpm_tis_spi_phy),
> -			   GFP_KERNEL);
> +	phy = devm_kzalloc(&dev->dev, sizeof(*phy), GFP_KERNEL);
>  	if (!phy)
>  		return -ENOMEM;
>  

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

RE: [PATCH 3/4] char/tpm: Improve a size determination in nine functions

[Alexander.Steffen]

> > Replace the specification of data structures by pointer dereferences
> > as the parameter for the operator "sizeof" to make the corresponding
> > size
> > determination a bit safer according to the Linux coding style
> > convention.
> 
> 
> This patch does one style in favor of the other.

I actually prefer that style, so I'd welcome this change :)

> At the end it's Jarkko's call, though I would NAK this as I think some
> one already told this to you for some other similar patch(es).
> 
> 
> I even would suggest to stop doing this noisy stuff, which keeps people
> busy for nothing.

Cleaning up old code is also worth something, even if does not change one bit in the assembly output in the end...

Alexander

Re: [PATCH 0/4] char-TPM: Adjustments for ten function implementations

[Mimi Zohar]

Hi Julia,

On Tue, 2017-10-17 at 12:11 +0200, Julia Lawall wrote:
> 
> On Tue, 17 Oct 2017, Dan Carpenter wrote:
> 
> > On Tue, Oct 17, 2017 at 10:56:42AM +0200, Julia Lawall wrote:
> > >
> > >
> > > On Tue, 17 Oct 2017, Dan Carpenter wrote:
> > >
> > > > On Mon, Oct 16, 2017 at 09:35:12PM +0300, Jarkko Sakkinen wrote:
> > > > >
> > > > > A minor complaint: all commits are missing "Fixes:" tag.
> > > > >
> > > >
> > > > Fixes is only for bug fixes.  These don't fix any bugs.
> > >
> > > 0-day seems to put Fixes for everything.  Should they be removed when the
> > > old code is undesirable but doesn't actually cause a crash, eg out of date
> > > API.
> >
> > Yeah, I feel like Fixes tags don't belong for API updates and cleanups.
> 
> OK, I will remove them from the patches that go through me where they
> don't seem appropriate.

The "Fixes" tag is an indication that the patch should be backported.
The requirements for what should be backported are pretty stringent. 

Mimi

Re: [PATCH 0/4] char-TPM: Adjustments for ten function implementations

[Michael Ellerman]

Dan Carpenter <dan.carpenter@oracle.com> writes:

> On Tue, Oct 17, 2017 at 10:56:42AM +0200, Julia Lawall wrote:
>> 
>> 
>> On Tue, 17 Oct 2017, Dan Carpenter wrote:
>> 
>> > On Mon, Oct 16, 2017 at 09:35:12PM +0300, Jarkko Sakkinen wrote:
>> > >
>> > > A minor complaint: all commits are missing "Fixes:" tag.
>> > >
>> >
>> > Fixes is only for bug fixes.  These don't fix any bugs.
>> 
>> 0-day seems to put Fixes for everything.  Should they be removed when the
>> old code is undesirable but doesn't actually cause a crash, eg out of date
>> API.
>
> Yeah, I feel like Fixes tags don't belong for API updates and cleanups.

I try to use the criteria of "if someone had backported commit A, would
they also want commit B" (where B Fixes: A).

So it's a bit broader than just "A had a *bug*" and this is the fix.

That's obviously still a bit of a slippery slope, but somewhat helpful I
think. eg, pretty much no one is interested in backporting spelling
fixes, so those aren't Fixes.

And generally people aren't interested in backporting commits like these
ones that just update coding style.

cheers

Re: [PATCH 3/4] char/tpm: Improve a size determination in nine functions

[Mimi Zohar]

On Tue, 2017-10-17 at 11:50 +0000, Alexander.Steffen@infineon.com
wrote:
> > > Replace the specification of data structures by pointer dereferences
> > > as the parameter for the operator "sizeof" to make the corresponding
> > > size
> > > determination a bit safer according to the Linux coding style
> > > convention.
> > 
> > 
> > This patch does one style in favor of the other.
> 
> I actually prefer that style, so I'd welcome this change :)

Style changes should be reviewed and documented, like any other code
change, and added to Documentation/process/coding-style.rst or an
equivalent file.

> > At the end it's Jarkko's call, though I would NAK this as I think some
> > one already told this to you for some other similar patch(es).
> > 
> > 
> > I even would suggest to stop doing this noisy stuff, which keeps people
> > busy for nothing.
> 
> Cleaning up old code is also worth something, even if does not
> change one bit in the assembly output in the end...

Wow, you're opening the door really wide for all sorts of trivial
changes!  Hope you have the time and inclination to review and comment
on all of them.  I certainly don't.

There is a major difference between adding these sorts of checks to
the tools in the scripts directory or even to the zero day bots that
catch different sorts of errors, BEFORE code is upstreamed, and
patches like these, after the fact.

After the code has been upstreamed, it is a lot more difficult to
justify changes like this.  It impacts both code that is being
developed AND backporting bug fixes.

Mimi

Re: [PATCH 3/4] char/tpm: Improve a size determination in nine functions

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 2335 bytes --]



On Tue, 17 Oct 2017, Mimi Zohar wrote:

> On Tue, 2017-10-17 at 11:50 +0000, Alexander.Steffen@infineon.com
> wrote:
> > > > Replace the specification of data structures by pointer dereferences
> > > > as the parameter for the operator "sizeof" to make the corresponding
> > > > size
> > > > determination a bit safer according to the Linux coding style
> > > > convention.
> > >
> > >
> > > This patch does one style in favor of the other.
> >
> > I actually prefer that style, so I'd welcome this change :)
>
> Style changes should be reviewed and documented, like any other code
> change, and added to Documentation/process/coding-style.rst or an
> equivalent file.

Actually, it has been there for many years:

14) Allocating memory
---------------------
...
The preferred form for passing a size of a struct is the following:

.. code-block:: c

	p = kmalloc(sizeof(*p), ...);

The alternative form where struct name is spelled out hurts readability and
introduces an opportunity for a bug when the pointer variable type is changed
but the corresponding sizeof that is passed to a memory allocator is not.

julia

>
> > > At the end it's Jarkko's call, though I would NAK this as I think some
> > > one already told this to you for some other similar patch(es).
> > >
> > >
> > > I even would suggest to stop doing this noisy stuff, which keeps people
> > > busy for nothing.
> >
> > Cleaning up old code is also worth something, even if does not
> > change one bit in the assembly output in the end...
>
> Wow, you're opening the door really wide for all sorts of trivial
> changes!  Hope you have the time and inclination to review and comment
> on all of them.  I certainly don't.
>
> There is a major difference between adding these sorts of checks to
> the tools in the scripts directory or even to the zero day bots that
> catch different sorts of errors, BEFORE code is upstreamed, and
> patches like these, after the fact.
>
> After the code has been upstreamed, it is a lot more difficult to
> justify changes like this.  It impacts both code that is being
> developed AND backporting bug fixes.
>
> Mimi
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 3/4] char/tpm: Improve a size determination in nine functions

[Andy Shevchenko]

On Tue, 2017-10-17 at 08:52 -0400, Mimi Zohar wrote:
> On Tue, 2017-10-17 at 11:50 +0000, Alexander.Steffen@infineon.com
> wrote:
> > > > Replace the specification of data structures by pointer
> > > > dereferences
> > > > as the parameter for the operator "sizeof" to make the
> > > > corresponding
> > > > size
> > > > determination a bit safer according to the Linux coding style
> > > > convention.
> > > 
> > > 
> > > This patch does one style in favor of the other.
> > 
> > I actually prefer that style, so I'd welcome this change :)
> 
> Style changes should be reviewed and documented, like any other code
> change, and added to Documentation/process/coding-style.rst or an
> equivalent file.

+1.

> > > At the end it's Jarkko's call, though I would NAK this as I think
> > > some
> > > one already told this to you for some other similar patch(es).
> > > 
> > > 
> > > I even would suggest to stop doing this noisy stuff, which keeps
> > > people
> > > busy for nothing.
> > 
> > Cleaning up old code is also worth something, even if does not
> > change one bit in the assembly output in the end...
> 
> Wow, you're opening the door really wide for all sorts of trivial
> changes!  Hope you have the time and inclination to review and comment
> on all of them.  I certainly don't.

Moreover and not so obvious is an open door for making back port of
*real* fixes much harder!

> There is a major difference between adding these sorts of checks to
> the tools in the scripts directory or even to the zero day bots that
> catch different sorts of errors, BEFORE code is upstreamed, and
> patches like these, after the fact.

+1.

> After the code has been upstreamed, it is a lot more difficult to
> justify changes like this.  It impacts both code that is being
> developed AND backporting bug fixes.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 3/4] char/tpm: Improve a size determination in nine functions

[Mimi Zohar]

On Tue, 2017-10-17 at 14:58 +0200, Julia Lawall wrote:
> 
> On Tue, 17 Oct 2017, Mimi Zohar wrote:
> 
> > On Tue, 2017-10-17 at 11:50 +0000, Alexander.Steffen@infineon.com
> > wrote:
> > > > > Replace the specification of data structures by pointer dereferences
> > > > > as the parameter for the operator "sizeof" to make the corresponding
> > > > > size
> > > > > determination a bit safer according to the Linux coding style
> > > > > convention.
> > > >
> > > >
> > > > This patch does one style in favor of the other.
> > >
> > > I actually prefer that style, so I'd welcome this change :)
> >
> > Style changes should be reviewed and documented, like any other code
> > change, and added to Documentation/process/coding-style.rst or an
> > equivalent file.
> 
> Actually, it has been there for many years:
> 
> 14) Allocating memory
> ---------------------
> ...
> The preferred form for passing a size of a struct is the following:
> 
> .. code-block:: c
> 
> 	p = kmalloc(sizeof(*p), ...);
> 
> The alternative form where struct name is spelled out hurts readability and
> introduces an opportunity for a bug when the pointer variable type is changed
> but the corresponding sizeof that is passed to a memory allocator is not.

True, thanks for the reminder.  Is this common in new code?  Is there
a script/ or some other automated way of catching this usage before
patches are upstreamed?

Just as you're doing here, the patch description should reference this
in the patch description.

Mimi

RE: [PATCH 3/4] char/tpm: Improve a size determination in nine functions

[Alexander.Steffen]

> On Tue, 2017-10-17 at 11:50 +0000, Alexander.Steffen@infineon.com
> wrote:
> > > > Replace the specification of data structures by pointer dereferences
> > > > as the parameter for the operator "sizeof" to make the corresponding
> > > > size
> > > > determination a bit safer according to the Linux coding style
> > > > convention.
> > >
> > >
> > > This patch does one style in favor of the other.
> >
> > I actually prefer that style, so I'd welcome this change :)
> 
> Style changes should be reviewed and documented, like any other code
> change, and added to Documentation/process/coding-style.rst or an
> equivalent file.
> 
> > > At the end it's Jarkko's call, though I would NAK this as I think some
> > > one already told this to you for some other similar patch(es).
> > >
> > >
> > > I even would suggest to stop doing this noisy stuff, which keeps people
> > > busy for nothing.
> >
> > Cleaning up old code is also worth something, even if does not
> > change one bit in the assembly output in the end...
> 
> Wow, you're opening the door really wide for all sorts of trivial
> changes!  Hope you have the time and inclination to review and comment
> on all of them.  I certainly don't.

Well, isn't the point of trivial changes that they are trivial to review? :) For things like that there is probably not even a need to run a test, though with sufficient automation that should not be a problem either.

> There is a major difference between adding these sorts of checks to
> the tools in the scripts directory or even to the zero day bots that
> catch different sorts of errors, BEFORE code is upstreamed, and
> patches like these, after the fact.

Catching those things early in the process is certainly preferable. But at some point you need to fix the existing code, or you'll end up with a mashup of different styles, just because you did not want to touch old code.

> After the code has been upstreamed, it is a lot more difficult to
> justify changes like this.  It impacts both code that is being
> developed AND backporting bug fixes.

Backporting could be an argument, but even that should not be allowed to block improvements indefinitely. I'd prefer a world in which the current code is nice and clean and easy to maintain, to a world where we never touch old code unless it is proven to be wrong.

But looking at the code in question, I cannot see how this should ever be a serious problem. Even when backporting a change takes now ten minutes instead of five, which means it is twice as hard, it is still not difficult.

Alexander

Re: [PATCH 3/4] char/tpm: Improve a size determination in nine functions

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 1918 bytes --]



On Tue, 17 Oct 2017, Mimi Zohar wrote:

> On Tue, 2017-10-17 at 14:58 +0200, Julia Lawall wrote:
> >
> > On Tue, 17 Oct 2017, Mimi Zohar wrote:
> >
> > > On Tue, 2017-10-17 at 11:50 +0000, Alexander.Steffen@infineon.com
> > > wrote:
> > > > > > Replace the specification of data structures by pointer dereferences
> > > > > > as the parameter for the operator "sizeof" to make the corresponding
> > > > > > size
> > > > > > determination a bit safer according to the Linux coding style
> > > > > > convention.
> > > > >
> > > > >
> > > > > This patch does one style in favor of the other.
> > > >
> > > > I actually prefer that style, so I'd welcome this change :)
> > >
> > > Style changes should be reviewed and documented, like any other code
> > > change, and added to Documentation/process/coding-style.rst or an
> > > equivalent file.
> >
> > Actually, it has been there for many years:
> >
> > 14) Allocating memory
> > ---------------------
> > ...
> > The preferred form for passing a size of a struct is the following:
> >
> > .. code-block:: c
> >
> > 	p = kmalloc(sizeof(*p), ...);
> >
> > The alternative form where struct name is spelled out hurts readability and
> > introduces an opportunity for a bug when the pointer variable type is changed
> > but the corresponding sizeof that is passed to a memory allocator is not.
>
> True, thanks for the reminder.  Is this common in new code?  Is there
> a script/ or some other automated way of catching this usage before
> patches are upstreamed?
>
> Just as you're doing here, the patch description should reference this
> in the patch description.

The comment in the documentation seems have been there since Linux 2.6.14,
ie 2005.  The fact that a lot of code still doesn't use that style, 12
years later, suggests that actually it is not preferred, or not preferred
by everyone.  Perhaps the paragraph in coding style should just be
dropped.

julia

Re: [PATCH 0/4] char-TPM: Adjustments for ten function implementations

[James Bottomley]

On Tue, 2017-10-17 at 11:25 +0200, SF Markus Elfring wrote:
> > 
> > Fixes is only for bug fixes.  These don't fix any bugs.
> 
> How do you distinguish these in questionable source code
> from other error categories or software weaknesses?

A style change is one that doesn't change the effect of the execution.
 These don't actually even change the assembly, so there's programmatic
proof they're not fixing anything.

Bug means potentially user visible fault.  In any bug fix commit you
should document the fault and its effects on users so those backporting
can decide if they care or not.

James

Re: [PATCH 0/4] char-TPM: Adjustments for ten function implementations

[SF Markus Elfring]

>>> Fixes is only for bug fixes.  These don't fix any bugs.
>>
>> How do you distinguish these in questionable source code
>> from other error categories or software weaknesses?
> 
> A style change is one that doesn't change the effect of the execution.

This can occasionally be fine, can't it?


>  These don't actually even change the assembly,

How did you check it?

I would expect that there are useful run time effects to consider
for three proposed update steps (in this patch series).


> so there's programmatic proof they're not fixing anything.

I find that the software refactoring “Improve a size determination in nine functions”
should fit to this observation (while the source code can become a bit better).


> Bug means potentially user visible fault.

Thanks for your constructive feedback.

Regards,
Markus

Re: char/tpm: Improve a size determination in nine functions

[SF Markus Elfring]

>> 	p = kmalloc(sizeof(*p), ...);
>>
>> The alternative form where struct name is spelled out hurts readability and
>> introduces an opportunity for a bug when the pointer variable type is changed
>> but the corresponding sizeof that is passed to a memory allocator is not.
> 
> True, thanks for the reminder.

Will it trigger further software development considerations (besides my contributions)?


> Is this common in new code?

Do you start an official survey here?


> Is there a script/ or some other automated way of catching this usage

Yes. - I am using an approach for the semantic patch language.   ;-)


> before patches are upstreamed?

I imagine that a corresponding source code analysis variant could be applied
in more cases if sufficient acceptance could be achieved.


> Just as you're doing here, the patch description should reference this
> in the patch description.

Do you find my wording “This issue was detected by using the Coccinelle software.” insufficient?

Regards,
Markus

Re: char/tpm: Improve a size determination in nine functions

[Mimi Zohar]

On Tue, 2017-10-17 at 20:41 +0200, SF Markus Elfring wrote:

> Do you find my wording “This issue was detected by using the
> Coccinelle software.” insufficient?

The question is not whether it is insufficient, but whether it is
appropriate.  Detecting Coccinelle issues is one step.  The next step
is deciding what to do with them.  Up to now, these messages have been
sent out as informational, not as patches.

Before sending patches to change existing code, address the "problem"
so that it doesn't continue to happen.  Only afterwards is it
appropriate to discuss what to do with existing code.

Mimi

Re: char/tpm: Improve a size determination in nine functions

[Andy Shevchenko]

On Tue, 2017-10-17 at 20:41 +0200, SF Markus Elfring wrote:
> > > 	p = kmalloc(sizeof(*p), ...);
> > > 
> > > The alternative form where struct name is spelled out hurts
> > > readability and
> > > introduces an opportunity for a bug when the pointer variable type
> > > is changed
> > > but the corresponding sizeof that is passed to a memory allocator
> > > is not.
> > 

> > before patches are upstreamed?
> 
> I imagine that a corresponding source code analysis variant could be
> applied
> in more cases if sufficient acceptance could be achieved.

So, then instead of still keeping people busy with this noise you better
start doing something like CI integration with that for *new* code?

I'm pretty sure you may also exercise your achievements on
drivers/staging where it would be honored.

Have you talked to Fengguang (0-day LKP)? Have you talked to Arnd (I
think he is related to kernel-ci)?

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: char/tpm: Improve a size determination in nine functions

[SF Markus Elfring]

>> Do you find my wording “This issue was detected by using the
>> Coccinelle software.” insufficient?
> 
> The question is not whether it is insufficient, but whether it is appropriate.

I am curious on how our corresponding discussion will evolve further.


> Detecting Coccinelle issues is one step.  The next step is deciding
> what to do with them.

Will the clarification achieve any more useful results?


> Up to now, these messages have been sent out as informational, not as patches.

I sent some update suggestions as patches also in this series (as usual).


> Before sending patches to change existing code, address the "problem"
> so that it doesn't continue to happen.

It might be very challenging to achieve such a goal.


> Only afterwards is it appropriate to discuss what to do with existing code.

I would prefer to get corresponding improvements in both areas in parallel
(if it is generally possible).

Regards,
Markus

Re: char/tpm: Improve a size determination in nine functions

[SF Markus Elfring]

>> I imagine that a corresponding source code analysis variant could be applied
>> in more cases if sufficient acceptance could be achieved.
> 
> So, then instead of still keeping people busy with this noise you better
> start doing something like CI integration with that for *new* code?

There are various software development challenges to consider.


> I'm pretty sure you may also exercise your achievements on
> drivers/staging where it would be honored.

I am waiting for several improvements also for software components
in this area for a while. Would you like to take another look
at these change possibilities?


> Have you talked to Fengguang (0-day LKP)?

Not directly for this topic so far.


> Have you talked to Arnd (I think he is related to kernel-ci)?

I am curious on how he will respond to remaining open issues.

Regards,
Markus

Re: [PATCH 0/4] char-TPM: Adjustments for ten function implementations

[Joe Perches]

On Tue, 2017-10-17 at 08:57 -0700, James Bottomley wrote:
> On Tue, 2017-10-17 at 11:25 +0200, SF Markus Elfring wrote:
> > > 
> > > Fixes is only for bug fixes.  These don't fix any bugs.
> > 
> > How do you distinguish these in questionable source code
> > from other error categories or software weaknesses?
> 
> A style change is one that doesn't change the effect of the execution.
>  These don't actually even change the assembly, so there's programmatic
> proof they're not fixing anything.

The printk removals do change the objects.

The value of that type of change is only for
resource limited systems.

printk type changes should generally not be
considered fixes.

> Bug means potentially user visible fault.  In any bug fix commit you
> should document the fault and its effects on users so those backporting
> can decide if they care or not.

Markus' changelogs leave much to be desired.

Re: [PATCH 0/4] char-TPM: Adjustments for ten function implementations

[Michael Ellerman]

Mimi Zohar <zohar@linux.vnet.ibm.com> writes:
> On Tue, 2017-10-17 at 12:11 +0200, Julia Lawall wrote:
>> On Tue, 17 Oct 2017, Dan Carpenter wrote:
>> > On Tue, Oct 17, 2017 at 10:56:42AM +0200, Julia Lawall wrote:
>> > > On Tue, 17 Oct 2017, Dan Carpenter wrote:
>> > > > On Mon, Oct 16, 2017 at 09:35:12PM +0300, Jarkko Sakkinen wrote:
>> > > > >
>> > > > > A minor complaint: all commits are missing "Fixes:" tag.
>> > > > >
>> > > >
>> > > > Fixes is only for bug fixes.  These don't fix any bugs.
>> > >
>> > > 0-day seems to put Fixes for everything.  Should they be removed when the
>> > > old code is undesirable but doesn't actually cause a crash, eg out of date
>> > > API.
>> >
>> > Yeah, I feel like Fixes tags don't belong for API updates and cleanups.
>> 
>> OK, I will remove them from the patches that go through me where they
>> don't seem appropriate.
>
> The "Fixes" tag is an indication that the patch should be backported.

No it's not that strong. It's an indication that the patch fixes another
commit, which may or may not mean it should be backported depending on
the preferences of the backporter. If it *does* need backporting then
the Fixes tag helps identify where it should go.

The doco is actually pretty well worded IMO:

  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst#n183

  If your patch fixes a bug in a specific commit, e.g. you found an issue using
  ``git bisect``, please use the 'Fixes:' tag with the first 12 characters of
  the SHA-1 ID, and the one line summary.

and:

  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst#n602

  A Fixes: tag indicates that the patch fixes an issue in a previous commit. It
  is used to make it easy to determine where a bug originated, which can help
  review a bug fix. This tag also assists the stable kernel team in determining
  which stable kernel versions should receive your fix. This is the preferred
  method for indicating a bug fixed by the patch. See :ref:`describe_changes`
  for more details.


cheers

Re: char-TPM: Adjustments for ten function implementations

[SF Markus Elfring]

> The printk removals do change the objects.
> 
> The value of that type of change is only for resource limited systems.

I imagine that such small code adjustments are also useful for other systems.


> Markus' changelogs leave much to be desired.

Would you like to help more to improve the provided information
for the shown change patterns?

Regards,
Markus

RE: [PATCH 3/4] char/tpm: Improve a size determination in nine functions

[Alexander.Steffen]

> On Tue, 17 Oct 2017, Mimi Zohar wrote:
> 
> > On Tue, 2017-10-17 at 14:58 +0200, Julia Lawall wrote:
> > >
> > > On Tue, 17 Oct 2017, Mimi Zohar wrote:
> > >
> > > > On Tue, 2017-10-17 at 11:50 +0000, Alexander.Steffen@infineon.com
> > > > wrote:
> > > > > > > Replace the specification of data structures by pointer
> dereferences
> > > > > > > as the parameter for the operator "sizeof" to make the
> corresponding
> > > > > > > size
> > > > > > > determination a bit safer according to the Linux coding style
> > > > > > > convention.
> > > > > >
> > > > > >
> > > > > > This patch does one style in favor of the other.
> > > > >
> > > > > I actually prefer that style, so I'd welcome this change :)
> > > >
> > > > Style changes should be reviewed and documented, like any other
> code
> > > > change, and added to Documentation/process/coding-style.rst or an
> > > > equivalent file.
> > >
> > > Actually, it has been there for many years:
> > >
> > > 14) Allocating memory
> > > ---------------------
> > > ...
> > > The preferred form for passing a size of a struct is the following:
> > >
> > > .. code-block:: c
> > >
> > > 	p = kmalloc(sizeof(*p), ...);
> > >
> > > The alternative form where struct name is spelled out hurts readability
> and
> > > introduces an opportunity for a bug when the pointer variable type is
> changed
> > > but the corresponding sizeof that is passed to a memory allocator is not.
> >
> > True, thanks for the reminder.  Is this common in new code?  Is there
> > a script/ or some other automated way of catching this usage before
> > patches are upstreamed?
> >
> > Just as you're doing here, the patch description should reference this
> > in the patch description.
> 
> The comment in the documentation seems have been there since Linux
> 2.6.14,
> ie 2005.  The fact that a lot of code still doesn't use that style, 12
> years later, suggests that actually it is not preferred, or not preferred
> by everyone.  Perhaps the paragraph in coding style should just be
> dropped.

Or maybe everyone just copied existing code, which did not follow that pattern, because nobody bothered to fix old code ;-)

(This is true at least for tpm_tis_spi, where I was involved in its creation.)

Alexander

Re: char-TPM: Adjustments for ten function implementations

[Joe Perches]

On Wed, 2017-10-18 at 11:00 +0200, SF Markus Elfring wrote:
> > The printk removals do change the objects.
> > 
> > The value of that type of change is only for resource limited systems.
> 
> I imagine that such small code adjustments are also useful for other systems.

Your imagination and mine differ.
Where do you _think_ it matters?

For instance, nothing about

	sizeof(type)
vs
	sizeof(*ptr)

makes it easier for a human to read the code.

This class of change now require a syntactic parser
to find instances of the use of type where previously
a grep or equivalent tool worked well.

> > Markus' changelogs leave much to be desired.
> 
> Would you like to help more to improve the provided information
> for the shown change patterns?

I've done that for you far too many times already.

Your changelogs need to detail _why_ something is being
done, not describe any tool used to perform or find a
particular instance of any change.

RE: char-TPM: Adjustments for ten function implementations

[Alexander.Steffen]

> On Wed, 2017-10-18 at 11:00 +0200, SF Markus Elfring wrote:
> > > The printk removals do change the objects.
> > >
> > > The value of that type of change is only for resource limited systems.
> >
> > I imagine that such small code adjustments are also useful for other
> systems.
> 
> Your imagination and mine differ.
> Where do you _think_ it matters?
> 
> For instance, nothing about
> 
> 	sizeof(type)
> vs
> 	sizeof(*ptr)
> 
> makes it easier for a human to read the code.

If it does not make it easier to read the code for you, then maybe you should consider that this might not be true for all humans. For me, it makes it much easier to see at a glance, that code like ptr=malloc(sizeof(*ptr)) is correct.

Alexander

Re: char-TPM: Adjustments for ten function implementations

[SF Markus Elfring]

>> I imagine that such small code adjustments are also useful for other systems.
> 
> Your imagination and mine differ.

This can generally be.


> Where do you _think_ it matters?

It seems that this discussion branch referred still to my cover letter
for possible changes in the TPM software area.

The four update steps (in this patch series) demonstrate different
change possibilities which could be desired.
Would you like to distinguish them a bit more?


> For instance, nothing about
> 
> 	sizeof(type)
> vs
> 	sizeof(*ptr)
> 
> makes it easier for a human to read the code.

I could agree to this view (in the general short form).
But nine statements became shorter in the concrete update suggestion
so that such a reduction could help the trained eyes
of some software developers and code reviewers.


> This class of change now require a syntactic parser
> to find instances of the use of type where previously
> a grep or equivalent tool worked well.

Does the Linux coding style convention prefer safety over this
data processing concern?


>>> Markus' changelogs leave much to be desired.
>>
>> Would you like to help more to improve the provided information
>> for the shown change patterns?
> 
> I've done that for you far too many times already.

I got an other impression.
You gave constructive feedback (also for me) occasionally.

There were a few cases where a desired agreement was not achieved so far.


> Your changelogs need to detail _why_ something is being done,

I could improve descriptions if involved information sources
could also become clearer and really safe.


> not describe any tool used to perform or find a
> particular instance of any change.

This part refers to a bit of attribution.

Regards,
Markus

RE: char-TPM: Adjustments for ten function implementations

[Julia Lawall]

On Wed, 18 Oct 2017, Alexander.Steffen@infineon.com wrote:

> > On Wed, 2017-10-18 at 11:00 +0200, SF Markus Elfring wrote:
> > > > The printk removals do change the objects.
> > > >
> > > > The value of that type of change is only for resource limited systems.
> > >
> > > I imagine that such small code adjustments are also useful for other
> > systems.
> >
> > Your imagination and mine differ.
> > Where do you _think_ it matters?
> >
> > For instance, nothing about
> >
> > 	sizeof(type)
> > vs
> > 	sizeof(*ptr)
> >
> > makes it easier for a human to read the code.
>
> If it does not make it easier to read the code for you, then maybe you
> should consider that this might not be true for all humans. For me, it
> makes it much easier to see at a glance, that code like
> ptr=malloc(sizeof(*ptr)) is correct.

I don't think there is a perfect solution.  The type argument to sizeof
could have the wrong type.  The expression argument to sizeof could be
missing the *.  Unpleasant consequences are possible in both cases.
Probably each maintainer has a style they prefer.  Perhaps it could be
useful to adjust the code to follow the dominant strategy, in cases where
there are a inconsistencies.  For example

if (...)
  x = foo1(sizeof(struct xtype));
else
  x = foo2(sizeof(*x));

might at least cause some unnecessary mental effort to process.

julia

Re: char-TPM: Adjustments for ten function implementations

[Joe Perches]

On Wed, 2017-10-18 at 12:00 +0200, Julia Lawall wrote:
> 
> On Wed, 18 Oct 2017, Alexander.Steffen@infineon.com wrote:
> 
> > > On Wed, 2017-10-18 at 11:00 +0200, SF Markus Elfring wrote:
> > > > > The printk removals do change the objects.
> > > > > 
> > > > > The value of that type of change is only for resource limited systems.
> > > > 
> > > > I imagine that such small code adjustments are also useful for other
> > > 
> > > systems.
> > > 
> > > Your imagination and mine differ.
> > > Where do you _think_ it matters?
> > > 
> > > For instance, nothing about
> > > 
> > > 	sizeof(type)
> > > vs
> > > 	sizeof(*ptr)
> > > 
> > > makes it easier for a human to read the code.
> > 
> > If it does not make it easier to read the code for you, then maybe you
> > should consider that this might not be true for all humans. For me, it
> > makes it much easier to see at a glance, that code like
> > ptr=malloc(sizeof(*ptr)) is correct.
> 
> I don't think there is a perfect solution.  The type argument to sizeof
> could have the wrong type.  The expression argument to sizeof could be
> missing the *.

Yup.

Today, even after all of Markus' patches for this style
conversion, there is still only ~2:1 preference for
	ptr = k.alloc(sizeof(*ptr))
over
	ptr = k.alloc(sizeof(struct foo))
in the kernel tree

Ugly grep follows:

$ grep -rohP --include=*.[ch] "\w+\s*=\s*[kv].alloc\s*\(\s*sizeof.*," * | \
  sed -r -e 's/(\w+)\s*=\s*[kv].alloc\s*\(\s*sizeof\s*\(\s*\*\s*\1\s*\)/foo = k.alloc(sizeof(*foo))/' \
         -e 's/(\w+)\s*=\s*[kv].alloc\s*\(\s*sizeof\s*\(\s*struct\s+\w+\s*\)/foo = k.alloc(sizeof(struct foo))/' | \
  sort | uniq -c | sort -rn | head -2
   6123 foo = k.alloc(sizeof(*foo)),
   3060 foo = k.alloc(sizeof(struct foo)),

> Unpleasant consequences are possible in both cases.

Yup.

> Probably each maintainer has a style they prefer.  Perhaps it could be
> useful to adjust the code to follow the dominant strategy, in cases where
> there are a inconsistencies.  For example
> 
> if (...)
>   x = foo1(sizeof(struct xtype));
> else
>   x = foo2(sizeof(*x));
> 
> might at least cause some unnecessary mental effort to process.

Sure, but perhaps _only_ when there are inconsistencies
in the same compilation unit.'

RE: char-TPM: Adjustments for ten function implementations

[Alexander.Steffen]

> On Wed, 18 Oct 2017, Alexander.Steffen@infineon.com wrote:
> 
> > > On Wed, 2017-10-18 at 11:00 +0200, SF Markus Elfring wrote:
> > > > > The printk removals do change the objects.
> > > > >
> > > > > The value of that type of change is only for resource limited systems.
> > > >
> > > > I imagine that such small code adjustments are also useful for other
> > > systems.
> > >
> > > Your imagination and mine differ.
> > > Where do you _think_ it matters?
> > >
> > > For instance, nothing about
> > >
> > > 	sizeof(type)
> > > vs
> > > 	sizeof(*ptr)
> > >
> > > makes it easier for a human to read the code.
> >
> > If it does not make it easier to read the code for you, then maybe you
> > should consider that this might not be true for all humans. For me, it
> > makes it much easier to see at a glance, that code like
> > ptr=malloc(sizeof(*ptr)) is correct.
> 
> I don't think there is a perfect solution.

Maybe. But for the second variant the correctness is easier to check, both mentally and programmatically, because there is no need for any context (the type of ptr does not matter).

> The type argument to sizeof
> could have the wrong type.  The expression argument to sizeof could be
> missing the *.  Unpleasant consequences are possible in both cases.
> Probably each maintainer has a style they prefer.  Perhaps it could be
> useful to adjust the code to follow the dominant strategy, in cases where
> there are a inconsistencies.

Certainly. At least within a file, there should be only one style.

> For example
> 
> if (...)
>   x = foo1(sizeof(struct xtype));
> else
>   x = foo2(sizeof(*x));
> 
> might at least cause some unnecessary mental effort to process.
> 
> julia

Alexander

Re: char-TPM: Adjustments for ten function implementations

[Joe Perches]

On Wed, 2017-10-18 at 10:44 +0000, Alexander.Steffen@infineon.com wrote:
> > For instance, nothing about
> > > > 	sizeof(type)
> > > > vs
> > > > 	sizeof(*ptr)
> > > > makes it easier for a human to read the code.
> > > 
> > > If it does not make it easier to read the code for you, then maybe you
> > > should consider that this might not be true for all humans. For me, it
> > > makes it much easier to see at a glance, that code like
> > > ptr=malloc(sizeof(*ptr)) is correct.
> > 
> > I don't think there is a perfect solution.
> 
> Maybe. But for the second variant the correctness is easier to check,

How often should
	ptr = alloc(sizeof(*ptr))
be
	ptr = alloc(sizeof(**ptr))

>  both mentally and programmatically, because there is no need for any context (the type of ptr does not matter).

Context matters.

Re: Adjusting further size determinations?

[SF Markus Elfring]

> Ugly grep follows:
> 
> $ grep -rohP --include=*.[ch] "\w+\s*=\s*[kv].alloc\s*\(\s*sizeof.*," * | \
>   sed -r -e 's/(\w+)\s*=\s*[kv].alloc\s*\(\s*sizeof\s*\(\s*\*\s*\1\s*\)/foo = k.alloc(sizeof(*foo))/' \
>          -e 's/(\w+)\s*=\s*[kv].alloc\s*\(\s*sizeof\s*\(\s*struct\s+\w+\s*\)/foo = k.alloc(sizeof(struct foo))/' | \
>   sort | uniq -c | sort -rn | head -2
>    6123 foo = k.alloc(sizeof(*foo)),
>    3060 foo = k.alloc(sizeof(struct foo)),

Would you like to get this ratio changed in any ways?

Available development tools could help to improve the software situation
in a desired direction, couldn't they?


>> Unpleasant consequences are possible in both cases.

How much do you care to reduce the failure probability further?

Regards,
Markus

RE: char-TPM: Adjustments for ten function implementations

[Alexander.Steffen]

> On Wed, 2017-10-18 at 10:44 +0000, Alexander.Steffen@infineon.com wrote:
> > > For instance, nothing about
> > > > > 	sizeof(type)
> > > > > vs
> > > > > 	sizeof(*ptr)
> > > > > makes it easier for a human to read the code.
> > > >
> > > > If it does not make it easier to read the code for you, then maybe you
> > > > should consider that this might not be true for all humans. For me, it
> > > > makes it much easier to see at a glance, that code like
> > > > ptr=malloc(sizeof(*ptr)) is correct.
> > >
> > > I don't think there is a perfect solution.
> >
> > Maybe. But for the second variant the correctness is easier to check,
> 
> How often should
> 	ptr = alloc(sizeof(*ptr))
> be
> 	ptr = alloc(sizeof(**ptr))

Never? Because in that case it probably should be *ptr=alloc(sizeof(**ptr)), unless you are doing something horrible ;-)

Alexander

Re: Adjusting further size determinations?

[Joe Perches]

On Wed, 2017-10-18 at 13:00 +0200, SF Markus Elfring wrote:
> > Ugly grep follows:
> > 
> > $ grep -rohP --include=*.[ch] "\w+\s*=\s*[kv].alloc\s*\(\s*sizeof.*," * | \
> >   sed -r -e 's/(\w+)\s*=\s*[kv].alloc\s*\(\s*sizeof\s*\(\s*\*\s*\1\s*\)/foo = k.alloc(sizeof(*foo))/' \
> >          -e 's/(\w+)\s*=\s*[kv].alloc\s*\(\s*sizeof\s*\(\s*struct\s+\w+\s*\)/foo = k.alloc(sizeof(struct foo))/' | \
> >   sort | uniq -c | sort -rn | head -2
> >    6123 foo = k.alloc(sizeof(*foo)),
> >    3060 foo = k.alloc(sizeof(struct foo)),
> 
> Would you like to get this ratio changed in any ways?

No.

> Available development tools could help to improve the software situation
> in a desired direction, couldn't they?
> > > Unpleasant consequences are possible in both cases.
> How much do you care to reduce the failure probability further?

Zero.

The alloc style is trivially useful for new code.
Existing code doesn't need change.

Re: Adjusting further size determinations?

[SF Markus Elfring]

>>>> Unpleasant consequences are possible in both cases.
>> How much do you care to reduce the failure probability further?
> 
> Zero.

I am interested to improve the software situation a bit more here.

Regards,
Markus

RE: Adjusting further size determinations?

[David Laight]

From: SF Markus Elfring
> >>>> Unpleasant consequences are possible in both cases.
> >> How much do you care to reduce the failure probability further?
> >
> > Zero.
> 
> I am interested to improve the software situation a bit more here.

There are probably better places to spend your time!

If you want 'security' for kmalloc() then:

#define KMALLOC_TYPE(flags) (type *)kmalloc(sizeof (type), flags)
#define KMALLOC(ptr, flags) *(ptr) = KMALLOC_TYPE(typeof *(ptr), flags)

and change:
	ptr = kmalloc(sizeof *ptr, flags);
to:
	KMALLOC(&ptr, flags);

But it is all churn for churn's sake.

	David

RE: Adjusting further size determinations?

[Julia Lawall]

On Wed, 18 Oct 2017, David Laight wrote:

> From: SF Markus Elfring
> > >>>> Unpleasant consequences are possible in both cases.
> > >> How much do you care to reduce the failure probability further?
> > >
> > > Zero.
> >
> > I am interested to improve the software situation a bit more here.
>
> There are probably better places to spend your time!
>
> If you want 'security' for kmalloc() then:
>
> #define KMALLOC_TYPE(flags) (type *)kmalloc(sizeof (type), flags)
> #define KMALLOC(ptr, flags) *(ptr) = KMALLOC_TYPE(typeof *(ptr), flags)
>
> and change:
> 	ptr = kmalloc(sizeof *ptr, flags);
> to:
> 	KMALLOC(&ptr, flags);
>
> But it is all churn for churn's sake.

Please don't.  Coccinelle won't find real problems with kmalloc any more
if this is done.

julia

Re: Adjusting further size determinations?

[SF Markus Elfring]

>> If you want 'security' for kmalloc() then:
>>
>> #define KMALLOC_TYPE(flags) (type *)kmalloc(sizeof (type), flags)
>> #define KMALLOC(ptr, flags) *(ptr) = KMALLOC_TYPE(typeof *(ptr), flags)

Such an approach might help.


>> and change:
>> 	ptr = kmalloc(sizeof *ptr, flags);
>> to:
>> 	KMALLOC(&ptr, flags);
>>
>> But it is all churn for churn's sake.
> 
> Please don't.

Interesting …


> Coccinelle won't find real problems with kmalloc any more if this is done.

The corresponding source code analysis will become different
(or more challenging) then. Are you still looking for related solutions?

Regards,
Markus

Re: [PATCH 3/4] char/tpm: Improve a size determination in nine functions

[Jarkko Sakkinen]

On Tue, Oct 17, 2017 at 02:03:02PM +0300, Andy Shevchenko wrote:
> On Mon, 2017-10-16 at 19:33 +0200, SF Markus Elfring wrote:
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > Date: Mon, 16 Oct 2017 18:28:17 +0200
> > 
> > Replace the specification of data structures by pointer dereferences
> > as the parameter for the operator "sizeof" to make the corresponding
> > size
> > determination a bit safer according to the Linux coding style
> > convention.
> 
> 
> This patch does one style in favor of the other.
> 
> At the end it's Jarkko's call, though I would NAK this as I think some
> one already told this to you for some other similar patch(es).
> 
> 
> I even would suggest to stop doing this noisy stuff, which keeps people
> busy for nothing.

I favor using "sizeof(*foo)" for pointers but as a part of a commit where
something useful is done to the corresponding line of code.

So, I would say it's a NAK.

/Jarkko

Re: [PATCH 3/4] char/tpm: Improve a size determination in nine functions

[Jarkko Sakkinen]

On Tue, Oct 17, 2017 at 11:50:05AM +0000, Alexander.Steffen@infineon.com wrote:
> > > Replace the specification of data structures by pointer dereferences
> > > as the parameter for the operator "sizeof" to make the corresponding
> > > size
> > > determination a bit safer according to the Linux coding style
> > > convention.
> > 
> > 
> > This patch does one style in favor of the other.
> 
> I actually prefer that style, so I'd welcome this change :)
> 
> > At the end it's Jarkko's call, though I would NAK this as I think some
> > one already told this to you for some other similar patch(es).
> > 
> > 
> > I even would suggest to stop doing this noisy stuff, which keeps people
> > busy for nothing.
> 
> Cleaning up old code is also worth something, even if does not change
> one bit in the assembly output in the end...
> 
> Alexander

Quite insignificant clean up it is that does more harm that gives any
benefit as any new change adds debt to backporting.

Anyway, this has been a useful patch set for me in the sense that I have
clearer picture now on discarding/accepting commits. One line minor
clean up will be from now on automatic NAK unless it causes a compiler
warning or some other visible side-effect.

/Jarkko

Re: [PATCH 3/4] char/tpm: Improve a size determination in nine functions

[Jarkko Sakkinen]

On Tue, Oct 17, 2017 at 04:02:05PM +0300, Andy Shevchenko wrote:
> On Tue, 2017-10-17 at 08:52 -0400, Mimi Zohar wrote:
> > On Tue, 2017-10-17 at 11:50 +0000, Alexander.Steffen@infineon.com
> > wrote:
> > > > > Replace the specification of data structures by pointer
> > > > > dereferences
> > > > > as the parameter for the operator "sizeof" to make the
> > > > > corresponding
> > > > > size
> > > > > determination a bit safer according to the Linux coding style
> > > > > convention.
> > > > 
> > > > 
> > > > This patch does one style in favor of the other.
> > > 
> > > I actually prefer that style, so I'd welcome this change :)
> > 
> > Style changes should be reviewed and documented, like any other code
> > change, and added to Documentation/process/coding-style.rst or an
> > equivalent file.
> 
> +1.
> 
> > > > At the end it's Jarkko's call, though I would NAK this as I think
> > > > some
> > > > one already told this to you for some other similar patch(es).
> > > > 
> > > > 
> > > > I even would suggest to stop doing this noisy stuff, which keeps
> > > > people
> > > > busy for nothing.
> > > 
> > > Cleaning up old code is also worth something, even if does not
> > > change one bit in the assembly output in the end...
> > 
> > Wow, you're opening the door really wide for all sorts of trivial
> > changes!  Hope you have the time and inclination to review and comment
> > on all of them.  I certainly don't.
> 
> Moreover and not so obvious is an open door for making back port of
> *real* fixes much harder!

Yes. This is really the key observation:

  A commit must have value above the cost of fixing a merge conflict.

/Jarkko

Re: char/tpm: Improve a size determination in nine functions

[Jarkko Sakkinen]

On Tue, Oct 17, 2017 at 08:41:04PM +0200, SF Markus Elfring wrote:
> Do you find my wording “This issue was detected by using the
> Coccinelle software.” insufficient?

This is fine for cover letter, not for the commits.

After your analysis software finds an issue you should manually analyze
what is wrong and document that to the commit message. This applies to
sparse, coccinelle or any other tool.

Tool-based commit messages are bad for commit history where as clean
description gives idea what was done (if you have to maintain a GIT
tree).

In my opinion tool is doing all the work but the part that you should do
is absent.

> Regards,
> Markus

/Jarkko

Re: [PATCH 0/4] char-TPM: Adjustments for ten function implementations

[Jarkko Sakkinen]

On Mon, Oct 16, 2017 at 10:44:18PM +0200, SF Markus Elfring wrote:
> > A minor complaint: all commits are missing "Fixes:" tag.
> 
> * Do you require it to be added to the commit messages?

I don't require it. It's part of the development process:

https://www.kernel.org/doc/html/v4.12/process/submitting-patches.html

> * Would you like to get a finer patch granularity then?

I don't understand what you are asking.

> * Do you find any more information missing?
> 
> Regards,
> Markus

I think I already answered to this in my earlier responses (commit
messages).

I probably won't take "sizeof(*foo)" type of change even if it
is a recommended style if that is the only useful thing that the
commit does.

/Jarkko

Re: [PATCH 0/4] char-TPM: Adjustments for ten function implementations

[Jarkko Sakkinen]

On Tue, Oct 17, 2017 at 12:44:34PM +0300, Dan Carpenter wrote:
> On Tue, Oct 17, 2017 at 10:56:42AM +0200, Julia Lawall wrote:
> > 
> > 
> > On Tue, 17 Oct 2017, Dan Carpenter wrote:
> > 
> > > On Mon, Oct 16, 2017 at 09:35:12PM +0300, Jarkko Sakkinen wrote:
> > > >
> > > > A minor complaint: all commits are missing "Fixes:" tag.
> > > >
> > >
> > > Fixes is only for bug fixes.  These don't fix any bugs.
> > 
> > 0-day seems to put Fixes for everything.  Should they be removed when the
> > old code is undesirable but doesn't actually cause a crash, eg out of date
> > API.
> 
> Yeah, I feel like Fixes tags don't belong for API updates and cleanups.
> 
> regards,
> dan carpenter

So breaking a rule documented in the style guide is not a bug? :-)

/Jarkko

Re: [PATCH 0/4] char-TPM: Adjustments for ten function implementations

[Jarkko Sakkinen]

On Tue, Oct 17, 2017 at 08:57:13AM -0700, James Bottomley wrote:
> On Tue, 2017-10-17 at 11:25 +0200, SF Markus Elfring wrote:
> > > 
> > > Fixes is only for bug fixes.  These don't fix any bugs.
> > 
> > How do you distinguish these in questionable source code
> > from other error categories or software weaknesses?
> 
> A style change is one that doesn't change the effect of the execution.
>  These don't actually even change the assembly, so there's programmatic
> proof they're not fixing anything.
> 
> Bug means potentially user visible fault.  In any bug fix commit you
> should document the fault and its effects on users so those backporting
> can decide if they care or not.
> 
> James

OK, I'll adjust my definition of a bug :-)

/Jarkko

Re: char/tpm: Improve a size determination in nine functions

[SF Markus Elfring]

>> Do you find my wording “This issue was detected by using the
>> Coccinelle software.” insufficient?
> 
> This is fine for cover letter, not for the commits.

I guess that there are more opinions available by other contributors
for this aspect.


> After your analysis software finds an issue you should manually analyze
> what is wrong

This view is generally fine.


> and document that to the commit message.

I tried it in a single paragraph so far (besides the reference
for the tool).


> This applies to sparse, coccinelle or any other tool.

I find that further possibilities can be considered.


> Tool-based commit messages are bad for commit history

I disagree to this view.


> where as clean description gives idea what was done
> (if you have to maintain a GIT tree).

How do you think about to offer any wording for an alternative
which you would find better?


> In my opinion tool is doing all the work but the part
> that you should do is absent.

Really?

Regards,
Markus

Re: char-TPM: Adjustments for ten function implementations

[SF Markus Elfring]

>>> A minor complaint: all commits are missing "Fixes:" tag.
>>
>> * Do you require it to be added to the commit messages?
> 
> I don't require it. It's part of the development process:
> 
> https://www.kernel.org/doc/html/v4.12/process/submitting-patches.html

Yes. - But other contributors pointed the detail out again
that not every change is qualified for using this tag.


>> * Would you like to get a finer patch granularity then?
> 
> I don't understand what you are asking.

If you would insist on the addition of this tag to all my commits
for the discussed patch series, I imagine that I would need to split
the update step “Improve a size determination in nine functions”
into smaller parts.


>> * Do you find any more information missing?
> 
> I think I already answered to this in my earlier responses
> (commit messages).

Partly.


> I probably won't take "sizeof(*foo)" type of change even if it
> is a recommended style if that is the only useful thing that the
> commit does.

How much do you care for the section “14) Allocating memory”
in the document “coding-style.rst” then?

Regards,
Markus

Re: char/tpm: Improve a size determination in nine functions

[Jarkko Sakkinen]

On Wed, Oct 18, 2017 at 05:22:19PM +0200, SF Markus Elfring wrote:
> >> Do you find my wording “This issue was detected by using the
> >> Coccinelle software.” insufficient?
> > 
> > This is fine for cover letter, not for the commits.
> 
> I guess that there are more opinions available by other contributors
> for this aspect.
> 
> 
> > After your analysis software finds an issue you should manually analyze
> > what is wrong
> 
> This view is generally fine.
> 
> 
> > and document that to the commit message.
> 
> I tried it in a single paragraph so far (besides the reference
> for the tool).
> 
> 
> > This applies to sparse, coccinelle or any other tool.
> 
> I find that further possibilities can be considered.
> 
> 
> > Tool-based commit messages are bad for commit history
> 
> I disagree to this view.
> 
> 
> > where as clean description gives idea what was done
> > (if you have to maintain a GIT tree).
> 
> How do you think about to offer any wording for an alternative
> which you would find better?
> 
> 
> > In my opinion tool is doing all the work but the part
> > that you should do is absent.
> 
> Really?
> 
> Regards,
> Markus

Commit message should just describe in plain text what you are doing
and why.

/Jarkko

Re: [PATCH 0/4] char-TPM: Adjustments for ten function implementations

[James Bottomley]

On Wed, 2017-10-18 at 18:10 +0300, Jarkko Sakkinen wrote:
> On Tue, Oct 17, 2017 at 08:57:13AM -0700, James Bottomley wrote:
> > 
> > On Tue, 2017-10-17 at 11:25 +0200, SF Markus Elfring wrote:
> > > 
> > > > 
> > > > 
> > > > Fixes is only for bug fixes.  These don't fix any bugs.
> > > 
> > > How do you distinguish these in questionable source code
> > > from other error categories or software weaknesses?
> > 
> > A style change is one that doesn't change the effect of the
> > execution.
> >  These don't actually even change the assembly, so there's
> > programmatic
> > proof they're not fixing anything.
> > 
> > Bug means potentially user visible fault.  In any bug fix commit
> > you
> > should document the fault and its effects on users so those
> > backporting
> > can decide if they care or not.
> > 
> > James
> 
> OK, I'll adjust my definition of a bug :-)

Subsystems are free to define bugs in any reasonable way.  However,
there are two things to note here:

   1. The style guide is just that, a guide; it's not hard and fast rules.
       That means that violations aren't bugs in the usual sense.
       However, new code should mostly follow it and if it doesn't, there
      should be a good reason to go against the guide which should be
      explained in the change log.
   2. The coding style evolves, so older drivers usually don't conform.
       Classifying coding style issues as bugs leads to tons of patches
      "fixing" older drivers, some of which actually end up breaking the
      drivers in subtle ways which take ages to be found (at least that's
      what we've seen in SCSI).

James

Re: char/tpm: Improve a size determination in nine functions

[SF Markus Elfring]

> Commit message should just describe in plain text what you are doing

Did other contributors find the wording “Replace …”


> and why.

and “… a bit safer according to the Linux coding style convention.”
sufficient often enough already?

Which description would you find more appropriate for this change pattern?

Regards,
Markus

Re: [PATCH 0/4] char-TPM: Adjustments for ten function implementations

[Jarkko Sakkinen]

On Wed, Oct 18, 2017 at 09:09:48AM -0700, James Bottomley wrote:
> On Wed, 2017-10-18 at 18:10 +0300, Jarkko Sakkinen wrote:
> > On Tue, Oct 17, 2017 at 08:57:13AM -0700, James Bottomley wrote:
> > > 
> > > On Tue, 2017-10-17 at 11:25 +0200, SF Markus Elfring wrote:
> > > > 
> > > > > 
> > > > > 
> > > > > Fixes is only for bug fixes.  These don't fix any bugs.
> > > > 
> > > > How do you distinguish these in questionable source code
> > > > from other error categories or software weaknesses?
> > > 
> > > A style change is one that doesn't change the effect of the
> > > execution.
> > >  These don't actually even change the assembly, so there's
> > > programmatic
> > > proof they're not fixing anything.
> > > 
> > > Bug means potentially user visible fault.  In any bug fix commit
> > > you
> > > should document the fault and its effects on users so those
> > > backporting
> > > can decide if they care or not.
> > > 
> > > James
> > 
> > OK, I'll adjust my definition of a bug :-)
> 
> Subsystems are free to define bugs in any reasonable way.  However,
> there are two things to note here:
> 
>    1. The style guide is just that, a guide; it's not hard and fast rules.
>        That means that violations aren't bugs in the usual sense.
>        However, new code should mostly follow it and if it doesn't, there
>       should be a good reason to go against the guide which should be
>       explained in the change log.
>    2. The coding style evolves, so older drivers usually don't conform.
>        Classifying coding style issues as bugs leads to tons of patches
>       "fixing" older drivers, some of which actually end up breaking the
>       drivers in subtle ways which take ages to be found (at least that's
>       what we've seen in SCSI).
> 
> James

Makes sense. Thanks for verbose explanation.

/Jarkko

Re: char/tpm: Improve a size determination in nine functions

[Jarkko Sakkinen]

On Wed, Oct 18, 2017 at 06:43:10PM +0200, SF Markus Elfring wrote:
> > Commit message should just describe in plain text what you are doing
> 
> Did other contributors find the wording “Replace …”
> 
> 
> > and why.
> 
> and “… a bit safer according to the Linux coding style convention.”
> sufficient often enough already?
> 
> Which description would you find more appropriate for this change pattern?
> 
> Regards,
> Markus

For 1/4 and 2/4: explain why the message can be omitted. Remove sentence
about Coccinelle. That's all.
3/4: definitive NAK, too much noise compared to value.
4/4: this a good commit message. Requires a Tested-by before can be
accepted, which I'm not able to give.

Hope this helps.

/Jarkko

Re: char/tpm: Improve a size determination in nine functions

[Jarkko Sakkinen]

On Wed, Oct 18, 2017 at 08:18:58PM +0300, Jarkko Sakkinen wrote:
> On Wed, Oct 18, 2017 at 06:43:10PM +0200, SF Markus Elfring wrote:
> > > Commit message should just describe in plain text what you are doing
> > 
> > Did other contributors find the wording “Replace …”
> > 
> > 
> > > and why.
> > 
> > and “… a bit safer according to the Linux coding style convention.”
> > sufficient often enough already?
> > 
> > Which description would you find more appropriate for this change pattern?
> > 
> > Regards,
> > Markus
> 
> For 1/4 and 2/4: explain why the message can be omitted. Remove sentence
> about Coccinelle. That's all.
> 3/4: definitive NAK, too much noise compared to value.
> 4/4: this a good commit message. Requires a Tested-by before can be
> accepted, which I'm not able to give.
> 
> Hope this helps.
> 
> /Jarkko

One more word of advice: send the three as separate patches. My guess is
that it takes a factor longer time to apply 4/4 than other patches
because there's more limited crowd who can test it.

Re: char/tpm: Improve a size determination in nine functions

[SF Markus Elfring]

> For 1/4 and 2/4: explain why the message can be omitted.

Why did you not reply directly with this request for the update steps
with the subject “Delete an error message for a failed memory allocation
in tpm_…()”?

https://patchwork.kernel.org/patch/10009405/
https://patchwork.kernel.org/patch/10009415/

I find that there can be difficulty to show an appropriate information
source for the reasonable explanation of this change pattern.


> Remove sentence about Coccinelle.

I got the impression that there is a bit of value in such
a kind of attribution.


> That's all.

I assume that there might be also some communication challenges involved.


> 3/4: definitive NAK, too much noise compared to value.

I tried to reduce deviations from the Linux coding style again.
You do not like such an attempt for this software area so far.


> 4/4: this a good commit message.

Why did you not reply directly with this feedback for the update step
“[PATCH 4/4] char/tpm: Less checks in tpm_ibmvtpm_probe() after error detection”?

https://patchwork.kernel.org/patch/10009429/
https://lkml.kernel.org/r/<09a2c3a1-1b10-507d-a866-258b570f6da1@users.sourceforge.net>


> Requires a Tested-by before can be accepted, which I'm not able to give.

I am curious on how this detail will evolve.

Regards,
Markus

Re: char/tpm: Improve a size determination in nine functions

[Jerry Snitselaar]

On Wed Oct 18 17, SF Markus Elfring wrote:
>> For 1/4 and 2/4: explain why the message can be omitted.
>
>Why did you not reply directly with this request for the update steps
>with the subject “Delete an error message for a failed memory allocation
>in tpm_…()”?
>
>https://patchwork.kernel.org/patch/10009405/
>https://patchwork.kernel.org/patch/10009415/
>
>I find that there can be difficulty to show an appropriate information
>source for the reasonable explanation of this change pattern.
>

Shouldn't this information source for the explanation be the
submitter? I'd hope they understand what it is they are submitting.

>
>> Remove sentence about Coccinelle.
>
>I got the impression that there is a bit of value in such
>a kind of attribution.
>
>
>> That's all.
>
>I assume that there might be also some communication challenges involved.
>
>
>> 3/4: definitive NAK, too much noise compared to value.
>
>I tried to reduce deviations from the Linux coding style again.
>You do not like such an attempt for this software area so far.
>
>
>> 4/4: this a good commit message.
>
>Why did you not reply directly with this feedback for the update step
>“[PATCH 4/4] char/tpm: Less checks in tpm_ibmvtpm_probe() after error detection”?
>
>https://patchwork.kernel.org/patch/10009429/
>https://lkml.kernel.org/r/<09a2c3a1-1b10-507d-a866-258b570f6da1@users.sourceforge.net>
>
>
>> Requires a Tested-by before can be accepted, which I'm not able to give.
>
>I am curious on how this detail will evolve.
>
>Regards,
>Markus

Re: char/tpm: Improve a size determination in nine functions

[SF Markus Elfring]

> One more word of advice: send the three as separate patches.

I do not see a need for an immediate resend at the moment.


> My guess is that it takes a factor longer time to apply 4/4
> than other patches because there's more limited crowd who can test it.

This is fine for me if somebody would like to integrate
this update suggestion at all.


How do you think about to separate replies better between affected
update steps?

Regards,
Markus

Re: char/tpm: Improve a size determination in nine functions

[Andy Shevchenko]

On Wed, 2017-10-18 at 19:48 +0200, SF Markus Elfring wrote:
> > For 1/4 and 2/4: explain why the message can be omitted.

> > That's all.
> 
> I assume that there might be also some communication challenges
> involved.
> 
> 
> > 3/4: definitive NAK, too much noise compared to value.
> 
> I tried to reduce deviations from the Linux coding style again.
> You do not like such an attempt for this software area so far.

The problem here in a time line or what comes first. Definitely, you are
trying to fix the code which _is_ upstream vs. the code which _might be_
upstream (exception is drivers/staging).

Why didn't you listen to what people are telling you?

Why are you spending too much time on little sense crap instead of doing
real fixes?

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: char/tpm: Delete an error message for a failed memory allocation in tpm_…()

[SF Markus Elfring]

>> Why did you not reply directly with this request for the update steps
>> with the subject “Delete an error message for a failed memory allocation
>> in tpm_…()”?
>>
>> https://patchwork.kernel.org/patch/10009405/
>> https://patchwork.kernel.org/patch/10009415/
>>
>> I find that there can be difficulty to show an appropriate information
>> source for the reasonable explanation of this change pattern.
>>
> 
> Shouldn't this information source for the explanation be the submitter?

I offered a bit of information. I agree that it could become better eventually.


> I'd hope they understand what it is they are submitting.

I do this to some degree.   ;-)

But I would appreciate if I could refer to a single Linux document
for this change pattern around questionable error messages.
Would a corresponding link for an accepted explanation in the documentation
be nice in this case?

Regards,
Markus

Re: char-TPM: Adjustments for ten function implementations

[Michal Suchánek]

On Wed, 18 Oct 2017 02:18:46 -0700
Joe Perches <joe@perches.com> wrote:

> On Wed, 2017-10-18 at 11:00 +0200, SF Markus Elfring wrote:
> > > The printk removals do change the objects.
> > > 
> > > The value of that type of change is only for resource limited
> > > systems.  
> > 
> > I imagine that such small code adjustments are also useful for
> > other systems.  
> 
> Your imagination and mine differ.
> Where do you _think_ it matters?
> 
> For instance, nothing about
> 
> 	sizeof(type)
> vs
> 	sizeof(*ptr)
> 
> makes it easier for a human to read the code.
> 

However, it makes it less error-prone to modify the code.

If you do ptr = malloc(sizeof(*ptr)) and later you change the type of
the pointer the code is still correct whereas ptr = malloc(sizeof(some
type) no longer is.

That is the reason the source analysis tool warns about this usage and
you do not really need any more explanation for *this* change.

The others are not so clear.

Thanks

Michal

Re: [PATCH 4/4] char/tpm: Less checks in tpm_ibmvtpm_probe() after error detection

[Michal Suchánek]

Hello,


On Mon, 16 Oct 2017 19:34:56 +0200
SF Markus Elfring <elfring@users.sourceforge.net> wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 16 Oct 2017 19:00:34 +0200
> 
> Two pointer checks could be repeated by the tpm_ibmvtpm_probe()
> function during error handling even if the relevant properties can be
> determined for the involved variables before by source code analysis.
> 
> * Return directly after a call of the function "kzalloc" failed
>   at the beginning.
> 
> * Adjust jump targets so that extra checks can be omitted at the end.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/char/tpm/tpm_ibmvtpm.c | 17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_ibmvtpm.c
> b/drivers/char/tpm/tpm_ibmvtpm.c index a4b462a77b99..b8dda7546f64
> 100644 --- a/drivers/char/tpm/tpm_ibmvtpm.c
> +++ b/drivers/char/tpm/tpm_ibmvtpm.c
> @@ -610,7 +610,7 @@ static int tpm_ibmvtpm_probe(struct vio_dev
> *vio_dev, 
>  	ibmvtpm = kzalloc(sizeof(*ibmvtpm), GFP_KERNEL);
>  	if (!ibmvtpm)
> -		goto cleanup;
> +		return -ENOMEM;

Just no.

I have seen many fixes that do inverse of this after a piece of code
allocating some more resources was added before code that returns
straight away because it is the first allocation in a function.

>  
>  	ibmvtpm->dev = dev;
>  	ibmvtpm->vdev = vio_dev;
> @@ -619,7 +619,7 @@ static int tpm_ibmvtpm_probe(struct vio_dev
> *vio_dev, crq_q->crq_addr = (struct ibmvtpm_crq
> *)get_zeroed_page(GFP_KERNEL); if (!crq_q->crq_addr) {
>  		dev_err(dev, "Unable to allocate memory for
> crq_addr\n");
> -		goto cleanup;
> +		goto free_tpm;
>  	}
>  
>  	crq_q->num_entry = CRQ_RES_BUF_SIZE /
> sizeof(*crq_q->crq_addr); @@ -629,7 +629,7 @@ static int
> tpm_ibmvtpm_probe(struct vio_dev *vio_dev, 
>  	if (dma_mapping_error(dev, ibmvtpm->crq_dma_handle)) {
>  		dev_err(dev, "dma mapping failed\n");
> -		goto cleanup;
> +		goto free_page;
>  	}
>  
>  	rc = plpar_hcall_norets(H_REG_CRQ, vio_dev->unit_address,
> @@ -683,13 +683,10 @@ static int tpm_ibmvtpm_probe(struct vio_dev
> *vio_dev, reg_crq_cleanup:
>  	dma_unmap_single(dev, ibmvtpm->crq_dma_handle,
> CRQ_RES_BUF_SIZE, DMA_BIDIRECTIONAL);
> -cleanup:
> -	if (ibmvtpm) {
> -		if (crq_q->crq_addr)
> -			free_page((unsigned long)crq_q->crq_addr);
> -		kfree(ibmvtpm);
> -	}
> -

I think a single cleanup section is better than many labels that just
avoid a single null check.

As long as you can tell easily which resources were already allocated
and need to be freed it is saner to keep only one cleanup section.

If the code doing the allocation is changed in the future the single
cleanup can stay whereas multiple labels have to be rewritten again.

Also just changing this just for the sake of code style does not seem
worth it whatever style you prefer.

Thanks

Michal

Re: char/tpm: Improve a size determination in nine functions

[Michal Suchánek]

On Wed, 18 Oct 2017 21:03:13 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Wed, 2017-10-18 at 19:48 +0200, SF Markus Elfring wrote:
> > > For 1/4 and 2/4: explain why the message can be omitted.  
> 
> > > That's all.  
> > 
> > I assume that there might be also some communication challenges
> > involved.
> > 
> >   
> > > 3/4: definitive NAK, too much noise compared to value.  
> > 
> > I tried to reduce deviations from the Linux coding style again.
> > You do not like such an attempt for this software area so far.  
> 
> The problem here in a time line or what comes first. Definitely, you
> are trying to fix the code which _is_ upstream vs. the code which
> _might be_ upstream (exception is drivers/staging).
> 
> Why didn't you listen to what people are telling you?
> 
> Why are you spending too much time on little sense crap instead of
> doing real fixes?
> 

People are free to spend their time on what they like.

Even if no commit of this series lands in mainline it has been useful
to clarify what is preferred style and what is useful fix.

Thanks

Michal

Re: char/tpm: Improve a size determination in nine functions

[Jarkko Sakkinen]

On Wed, Oct 18, 2017 at 07:48:06PM +0200, SF Markus Elfring wrote:
> > For 1/4 and 2/4: explain why the message can be omitted.
> 
> Why did you not reply directly with this request for the update steps
> with the subject “Delete an error message for a failed memory allocation
> in tpm_…()”?
> 
> https://patchwork.kernel.org/patch/10009405/
> https://patchwork.kernel.org/patch/10009415/
> 
> I find that there can be difficulty to show an appropriate information
> source for the reasonable explanation of this change pattern.
> 
> 
> > Remove sentence about Coccinelle.
> 
> I got the impression that there is a bit of value in such
> a kind of attribution.
> 
> 
> > That's all.
> 
> I assume that there might be also some communication challenges involved.
> 
> 
> > 3/4: definitive NAK, too much noise compared to value.
> 
> I tried to reduce deviations from the Linux coding style again.
> You do not like such an attempt for this software area so far.
> 
> 
> > 4/4: this a good commit message.
> 
> Why did you not reply directly with this feedback for the update step
> “[PATCH 4/4] char/tpm: Less checks in tpm_ibmvtpm_probe() after error detection”?
> 
> https://patchwork.kernel.org/patch/10009429/
> https://lkml.kernel.org/r/<09a2c3a1-1b10-507d-a866-258b570f6da1@users.sourceforge.net>
> 
> 
> > Requires a Tested-by before can be accepted, which I'm not able to give.
> 
> I am curious on how this detail will evolve.
> 
> Regards,
> Markus

I've given clear enough instructions what to do with the commits. This
is the point where I stop caring about this mail thread. Thank you.

/Jarkko

Re: [PATCH 4/4] char/tpm: Less checks in tpm_ibmvtpm_probe() after error detection

[SF Markus Elfring]

>> @@ -683,13 +683,10 @@ static int tpm_ibmvtpm_probe(struct vio_dev *vio_dev,
>> reg_crq_cleanup:
>>  	dma_unmap_single(dev, ibmvtpm->crq_dma_handle,
>> CRQ_RES_BUF_SIZE, DMA_BIDIRECTIONAL);
>> -cleanup:
>> -	if (ibmvtpm) {
>> -		if (crq_q->crq_addr)
>> -			free_page((unsigned long)crq_q->crq_addr);
>> -		kfree(ibmvtpm);
>> -	}
>> -
> 
> I think a single cleanup section is better than many labels that just
> avoid a single null check.

I proposed to delete two unnecessary condition checks together with
an adjustment of jump targets.

Regards,
Markus

Re: [PATCH 4/4] char/tpm: Less checks in tpm_ibmvtpm_probe() after error detection

[Michal Suchánek]

On Thu, 19 Oct 2017 14:36:09 +0200
SF Markus Elfring <elfring@users.sourceforge.net> wrote:

> >> @@ -683,13 +683,10 @@ static int tpm_ibmvtpm_probe(struct vio_dev
> >> *vio_dev, reg_crq_cleanup:
> >>  	dma_unmap_single(dev, ibmvtpm->crq_dma_handle,
> >> CRQ_RES_BUF_SIZE, DMA_BIDIRECTIONAL);
> >> -cleanup:
> >> -	if (ibmvtpm) {
> >> -		if (crq_q->crq_addr)
> >> -			free_page((unsigned long)crq_q->crq_addr);
> >> -		kfree(ibmvtpm);
> >> -	}
> >> -  
> > 
> > I think a single cleanup section is better than many labels that
> > just avoid a single null check.  
> 
> I proposed to delete two unnecessary condition checks together with
> an adjustment of jump targets.
> 

They are necessary to ensure the code works with single jump target.

The compiler is free to optimize them away and create the new jump
target implicitly. Do not do the optimization in place of the compiler.
It can do it automatically, in most cases better, and automatically
adapt it to code changes.

Thanks

Michal

Re: [PATCH 0/4] char-TPM: Adjustments for ten function implementations

[Mimi Zohar]

On Wed, 2017-10-18 at 14:18 +1100, Michael Ellerman wrote:
> Mimi Zohar <zohar@linux.vnet.ibm.com> writes:
> > On Tue, 2017-10-17 at 12:11 +0200, Julia Lawall wrote:
> >> On Tue, 17 Oct 2017, Dan Carpenter wrote:
> >> > On Tue, Oct 17, 2017 at 10:56:42AM +0200, Julia Lawall wrote:
> >> > > On Tue, 17 Oct 2017, Dan Carpenter wrote:
> >> > > > On Mon, Oct 16, 2017 at 09:35:12PM +0300, Jarkko Sakkinen wrote:
> >> > > > >
> >> > > > > A minor complaint: all commits are missing "Fixes:" tag.
> >> > > > >
> >> > > >
> >> > > > Fixes is only for bug fixes.  These don't fix any bugs.
> >> > >
> >> > > 0-day seems to put Fixes for everything.  Should they be removed when the
> >> > > old code is undesirable but doesn't actually cause a crash, eg out of date
> >> > > API.
> >> >
> >> > Yeah, I feel like Fixes tags don't belong for API updates and cleanups.
> >> 
> >> OK, I will remove them from the patches that go through me where they
> >> don't seem appropriate.
> >
> > The "Fixes" tag is an indication that the patch should be backported.
> 
> No it's not that strong. It's an indication that the patch fixes another
> commit, which may or may not mean it should be backported depending on
> the preferences of the backporter. If it *does* need backporting then
> the Fixes tag helps identify where it should go.

Thank you for setting the record straight.

> The doco is actually pretty well worded IMO:
> 
>   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst#n183
> 
>   If your patch fixes a bug in a specific commit, e.g. you found an issue using
>   ``git bisect``, please use the 'Fixes:' tag with the first 12 characters of
>   the SHA-1 ID, and the one line summary.
> 
> and:
> 
>   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst#n602
> 
>   A Fixes: tag indicates that the patch fixes an issue in a previous commit. It
>   is used to make it easy to determine where a bug originated, which can help
>   review a bug fix. This tag also assists the stable kernel team in determining
>   which stable kernel versions should receive your fix. This is the preferred
>   method for indicating a bug fixed by the patch. See :ref:`describe_changes`
>   for more details.
> 
> 
> cheers
> 

Re: [PATCH 4/4] char/tpm: Less checks in tpm_ibmvtpm_probe() after error detection

[Dan Carpenter]

On Thu, Oct 19, 2017 at 01:56:32PM +0200, Michal Suchánek wrote:
> 
> I think a single cleanup section is better than many labels that just
> avoid a single null check.
>

I am not a big advocate of churn, but one err style error handling is
really bug prone.

I'm dealing with static analysis so most of the bugs I see are error
handling bugs.  That's because error handling is hard to test but easy
for static analysis.  One err style error handling is the worst because
you get things like:

fail:
	kfree(foo->bar);
	kfree(foo);

Oops, foo->bar is a NULL dereference.  And generally, it's a bad thing
to free things that haven't been allocated so, for example, I see
refcounting bugs in error handling paths as well where we decrement
something that wasn't incremented.  Freeing everything is more
complicated than just freeing one specific thing the way standard kernel
error handling works.

> As long as you can tell easily which resources were already allocated
> and need to be freed it is saner to keep only one cleanup section.
> 

Sure, if the function is simple and short then the error handling is
normally simple and short.  This is true for any style of error
handling.

> If the code doing the allocation is changed in the future the single
> cleanup can stay whereas multiple labels have to be rewritten again.

No, they don't unless you choose bad label names.  Perhaps numbered
labels?  We don't get a lot of those in the kernel any more.  Label
name should be based on what the label does.  Often I see bad label
names like generic labels:

	foo = kmalloc();
	if (!foo)
		goto out;

What is out going to do?  Another common anti-pattern is come-from
labels:

	foo = kmalloc();
	if (!foo)
		goto kmalloc_failed;

Obviously, we can see from the if statement that the alloc failed and
you *just* know the next line is going to be is going to be:

	if (invalid)
		goto kmalloc_failed;

Which is wrong because kmalloc didn't fail...  But if the label name is
based on what it does then, when you add or a remove an allocation, you
just have to edit the one thing.  It's very simple:

+	foo = new_alloc();
+	if (!foo)
+		return -ENOMEM;
+
 	bar = old_alloc();
-	if (!bar)
-		return -ENOMEM;
+	if (!bar) {
+		ret -ENOMEM;
+		goto free_foo;

[ snip ]

 free_whatever:
 	free(whatever);
+free_foo:
+	free(foo);

 return ret;

> 
> Also just changing this just for the sake of code style does not seem
> worth it whatever style you prefer.

True.

regards,
dan carpenter

Re: [PATCH 4/4] char/tpm: Less checks in tpm_ibmvtpm_probe() after error detection

[Michal Suchánek]

On Thu, 19 Oct 2017 16:36:46 +0300
Dan Carpenter <dan.carpenter@oracle.com> wrote:

> On Thu, Oct 19, 2017 at 01:56:32PM +0200, Michal Suchánek wrote:
> > 
> > I think a single cleanup section is better than many labels that
> > just avoid a single null check.
> >  
> 
> I am not a big advocate of churn, but one err style error handling is
> really bug prone.
> 
> I'm dealing with static analysis so most of the bugs I see are error
> handling bugs.  

But it looks like you do not deal much with actual code development
because then you would know that some of the changes proposed by the
static analysis lead to errors later when the code dynamically changes.

> That's because error handling is hard to test but easy
> for static analysis.  One err style error handling is the worst
> because you get things like:
> 
> fail:
> 	kfree(foo->bar);
> 	kfree(foo);
> 
> Oops, foo->bar is a NULL dereference.  And generally, it's a bad thing
> to free things that haven't been allocated so, for example, I see
> refcounting bugs in error handling paths as well where we decrement
> something that wasn't incremented.  Freeing everything is more
> complicated than just freeing one specific thing the way standard
> kernel error handling works.

It depends on the function in question. If it only allocates memory
which is not reference-counted and kfree() checks for the null in most
cases it is easier to do just one big cleanup. 

If it is more complex more labels are preferable.

> 
> > As long as you can tell easily which resources were already
> > allocated and need to be freed it is saner to keep only one cleanup
> > section. 
> 
> Sure, if the function is simple and short then the error handling is
> normally simple and short.  This is true for any style of error
> handling.
> 
> > If the code doing the allocation is changed in the future the single
> > cleanup can stay whereas multiple labels have to be rewritten
> > again.  
> 
> No, they don't unless you choose bad label names.  

You obviously miss the fact that resource allocation is not always
added at the end of the function.

You may need to reorder the code and hence the order of allocation or
add allocation at the start. Both of these cases break multiple labels
and special cases but work with one big cleanup just fine.

Thanks

Michal

Re: [PATCH 4/4] char/tpm: Less checks in tpm_ibmvtpm_probe() after error detection

[Dan Carpenter]

tpm_ibmvtpm_probe() is an example of poor names.  It has the generic
ones like "cleanup" which don't say *what* is cleaned and the come-from
ones like "init_irq_cleanup" which don't say anything useful at all:

   647          rc = request_irq(vio_dev->irq, ibmvtpm_interrupt, 0,
   648                           tpm_ibmvtpm_driver_name, ibmvtpm);
   649          if (rc) {
   650                  dev_err(dev, "Error %d register irq 0x%x\n", rc, vio_dev->irq);
   651                  goto init_irq_cleanup;
   652          }
   653  
   654          rc = vio_enable_interrupts(vio_dev);
   655          if (rc) {
   656                  dev_err(dev, "Error %d enabling interrupts\n", rc);
   657                  goto init_irq_cleanup;
   658          }

Sadly, we never do call free_irq().

> It can do it automatically, in most cases better, and automatically
> adapt it to code changes.

I've heard this before that if you only have one label that does
everything then it's "automatic" and "future proof".  It's not true.
The best error handling is methodical error handling:

1) In the reverse order from how things were allocated
2) That mirrors the allocations exactly
3) That frees one thing at a time
4) With a proper, useful, readable label name which says what the goto
   does
5) That doesn't free anything which hasn't been allocated

regards,
dan carpenter

Re: [PATCH 4/4] char/tpm: Less checks in tpm_ibmvtpm_probe() after error detection

[Dan Carpenter]

On Thu, Oct 19, 2017 at 04:16:37PM +0200, Michal Suchánek wrote:
> On Thu, 19 Oct 2017 16:36:46 +0300
> Dan Carpenter <dan.carpenter@oracle.com> wrote:
> 
> > On Thu, Oct 19, 2017 at 01:56:32PM +0200, Michal Suchánek wrote:
> > > 
> > > I think a single cleanup section is better than many labels that
> > > just avoid a single null check.
> > >  
> > 
> > I am not a big advocate of churn, but one err style error handling is
> > really bug prone.
> > 
> > I'm dealing with static analysis so most of the bugs I see are error
> > handling bugs.  
> 
> But it looks like you do not deal much with actual code development
> because then you would know that some of the changes proposed by the
> static analysis lead to errors later when the code dynamically changes.
> 

This is silly...  Anyway, my record is there in git.  I mostly send
bugfixes, and not cleanups.  In terms of patches that are merged, I
probably have introduced one or two runtime bugs per year over the past
decade.

> > That's because error handling is hard to test but easy
> > for static analysis.  One err style error handling is the worst
> > because you get things like:
> > 
> > fail:
> > 	kfree(foo->bar);
> > 	kfree(foo);
> > 
> > Oops, foo->bar is a NULL dereference.  And generally, it's a bad thing
> > to free things that haven't been allocated so, for example, I see
> > refcounting bugs in error handling paths as well where we decrement
> > something that wasn't incremented.  Freeing everything is more
> > complicated than just freeing one specific thing the way standard
> > kernel error handling works.
> 
> It depends on the function in question. If it only allocates memory
> which is not reference-counted and kfree() checks for the null in most
> cases it is easier to do just one big cleanup.
> 

No.  Just always do it standard way.  If there is only one error
condition just free it directly:

	if (invalid) {
		free(foo);
		return -EINVAL;
	}

But once you add a second error condition then use gotos.  There is no
reason to deviate from the standard.

> If it is more complex more labels are preferable.
> 
> > 
> > > As long as you can tell easily which resources were already
> > > allocated and need to be freed it is saner to keep only one cleanup
> > > section. 
> > 
> > Sure, if the function is simple and short then the error handling is
> > normally simple and short.  This is true for any style of error
> > handling.
> > 
> > > If the code doing the allocation is changed in the future the single
> > > cleanup can stay whereas multiple labels have to be rewritten
> > > again.  
> > 
> > No, they don't unless you choose bad label names.  
> 
> You obviously miss the fact that resource allocation is not always
> added at the end of the function.
> 

No, in my example, the new allocation was added to the *start* not the
end.  It doesn't matter though, standard error handling works the same
either way.

> You may need to reorder the code and hence the order of allocation or
> add allocation at the start. Both of these cases break multiple labels
> and special cases but work with one big cleanup just fine.

No, You don't need to re-order the labels or change them.  The label
name says what is freed.  The order is the reverse order from how they
were allocated.  It's very simple.  You just have to keep track of the
most recently allocated thing:

	foo = alloc();
	if (!foo)
		return -ENOMEM;

	bar = alloc();
	if (!bar)
		goto free_foo;

	baz = alloc();
	if (!baz)
		goto free_bar;

You can tell this code doesn't leak just from looking at the label
names.

regards,
dan carpenter

Re: Circumstances for using the tag “Fixes” (or not)

[SF Markus Elfring]

>>> The "Fixes" tag is an indication that the patch should be backported.
>>
>> No it's not that strong. It's an indication that the patch fixes another
>> commit, which may or may not mean it should be backported depending on
>> the preferences of the backporter. If it *does* need backporting then
>> the Fixes tag helps identify where it should go.
> 
> Thank you for setting the record straight.
> 
>> The doco is actually pretty well worded IMO:

It seems that there are still interpretations left over for
further clarification.

Would any porters dare to distribute the deletion of questionable
condition checks (and special error messages) to more software versions?


>>   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst#n183
>>
>>   If your patch fixes a bug in a specific commit, e.g. you found an issue using
>>   ``git bisect``, please use the 'Fixes:' tag with the first 12 characters of
>>   the SHA-1 ID, and the one line summary.

Would you dare to categorise any software inefficiencies with a bug type?

Regards,
Markus

RE: [PATCH 3/4] char/tpm: Improve a size determination in nine functions

[Alexander.Steffen]

> On Tue, Oct 17, 2017 at 11:50:05AM +0000, Alexander.Steffen@infineon.com
> wrote:
> > > > Replace the specification of data structures by pointer dereferences
> > > > as the parameter for the operator "sizeof" to make the corresponding
> > > > size
> > > > determination a bit safer according to the Linux coding style
> > > > convention.
> > >
> > >
> > > This patch does one style in favor of the other.
> >
> > I actually prefer that style, so I'd welcome this change :)
> >
> > > At the end it's Jarkko's call, though I would NAK this as I think some
> > > one already told this to you for some other similar patch(es).
> > >
> > >
> > > I even would suggest to stop doing this noisy stuff, which keeps people
> > > busy for nothing.
> >
> > Cleaning up old code is also worth something, even if does not change
> > one bit in the assembly output in the end...
> >
> > Alexander
> 
> Quite insignificant clean up it is that does more harm that gives any
> benefit as any new change adds debt to backporting.
> 
> Anyway, this has been a useful patch set for me in the sense that I have
> clearer picture now on discarding/accepting commits.

Indeed. I have now a better understanding for why some code looks as ugly as it does.

> One line minor
> clean up will be from now on automatic NAK unless it causes a compiler
> warning or some other visible side-effect.

Not a nice policy, but at least a policy. I have deleted the tasks that I had still planned for other cleanup activities.

Alexander

Re: char/tpm: Less checks in tpm_ibmvtpm_probe() after error detection

[SF Markus Elfring]

>> If the code doing the allocation is changed in the future the single
>> cleanup can stay whereas multiple labels have to be rewritten again.
> 
> No, they don't unless you choose bad label names.  Perhaps numbered
> labels?  We don't get a lot of those in the kernel any more.  Label
> name should be based on what the label does.  Often I see bad label
> names like generic labels:
> 
> 	foo = kmalloc();
> 	if (!foo)
> 		goto out;
> 
> What is out going to do?  Another common anti-pattern is come-from
> labels:
> 
> 	foo = kmalloc();
> 	if (!foo)
> 		goto kmalloc_failed;
> 
> Obviously, we can see from the if statement that the alloc failed and
> you *just* know the next line is going to be is going to be:
> 
> 	if (invalid)
> 		goto kmalloc_failed;
> 
> Which is wrong because kmalloc didn't fail...  But if the label name is
> based on what it does then, when you add or a remove an allocation, you
> just have to edit the one thing.

Would you be interested in an update on a topic like “Source code review
around jump label usage”?
https://lkml.org/lkml/2015/12/11/378

Regards,
Markus

Re: [PATCH 3/4] char/tpm: Improve a size determination in nine functions

[Jarkko Sakkinen]

On Thu, Oct 19, 2017 at 04:58:23PM +0000, Alexander.Steffen@infineon.com wrote:
> > On Tue, Oct 17, 2017 at 11:50:05AM +0000, Alexander.Steffen@infineon.com
> > wrote:
> > > > > Replace the specification of data structures by pointer dereferences
> > > > > as the parameter for the operator "sizeof" to make the corresponding
> > > > > size
> > > > > determination a bit safer according to the Linux coding style
> > > > > convention.
> > > >
> > > >
> > > > This patch does one style in favor of the other.
> > >
> > > I actually prefer that style, so I'd welcome this change :)
> > >
> > > > At the end it's Jarkko's call, though I would NAK this as I think some
> > > > one already told this to you for some other similar patch(es).
> > > >
> > > >
> > > > I even would suggest to stop doing this noisy stuff, which keeps people
> > > > busy for nothing.
> > >
> > > Cleaning up old code is also worth something, even if does not change
> > > one bit in the assembly output in the end...
> > >
> > > Alexander
> > 
> > Quite insignificant clean up it is that does more harm that gives any
> > benefit as any new change adds debt to backporting.
> > 
> > Anyway, this has been a useful patch set for me in the sense that I have
> > clearer picture now on discarding/accepting commits.
> 
> Indeed. I have now a better understanding for why some code looks as
> ugly as it does.
> 
> > One line minor
> > clean up will be from now on automatic NAK unless it causes a compiler
> > warning or some other visible side-effect.
> 
> Not a nice policy, but at least a policy. I have deleted the tasks
> that I had still planned for other cleanup activities.
> 
> Alexander

1/4 and 2/4 are sensible clean ups as long as the commit message is
refined.

Moving more functions to use tpm_buf instead of nasty tpm_cmd_t are
also welcome changes.

Documenting functions (exported mainly) is also welcome. Or refining
documentation.

It's really case by case. The important thing in small clean ups is
a clearly written commit message that explains rationale.

/Jarkko

Re: [PATCH 3/4] char/tpm: Improve a size determination in nine functions

[Jarkko Sakkinen]

On Fri, Oct 20, 2017 at 12:01:39PM +0300, Jarkko Sakkinen wrote:
> On Thu, Oct 19, 2017 at 04:58:23PM +0000, Alexander.Steffen@infineon.com wrote:
> > > On Tue, Oct 17, 2017 at 11:50:05AM +0000, Alexander.Steffen@infineon.com
> > > wrote:
> > > > > > Replace the specification of data structures by pointer dereferences
> > > > > > as the parameter for the operator "sizeof" to make the corresponding
> > > > > > size
> > > > > > determination a bit safer according to the Linux coding style
> > > > > > convention.
> > > > >
> > > > >
> > > > > This patch does one style in favor of the other.
> > > >
> > > > I actually prefer that style, so I'd welcome this change :)
> > > >
> > > > > At the end it's Jarkko's call, though I would NAK this as I think some
> > > > > one already told this to you for some other similar patch(es).
> > > > >
> > > > >
> > > > > I even would suggest to stop doing this noisy stuff, which keeps people
> > > > > busy for nothing.
> > > >
> > > > Cleaning up old code is also worth something, even if does not change
> > > > one bit in the assembly output in the end...
> > > >
> > > > Alexander
> > > 
> > > Quite insignificant clean up it is that does more harm that gives any
> > > benefit as any new change adds debt to backporting.
> > > 
> > > Anyway, this has been a useful patch set for me in the sense that I have
> > > clearer picture now on discarding/accepting commits.
> > 
> > Indeed. I have now a better understanding for why some code looks as
> > ugly as it does.
> > 
> > > One line minor
> > > clean up will be from now on automatic NAK unless it causes a compiler
> > > warning or some other visible side-effect.
> > 
> > Not a nice policy, but at least a policy. I have deleted the tasks
> > that I had still planned for other cleanup activities.
> > 
> > Alexander
> 
> 1/4 and 2/4 are sensible clean ups as long as the commit message is
> refined.
> 
> Moving more functions to use tpm_buf instead of nasty tpm_cmd_t are
> also welcome changes.
> 
> Documenting functions (exported mainly) is also welcome. Or refining
> documentation.
> 
> It's really case by case. The important thing in small clean ups is
> a clearly written commit message that explains rationale.
> 
> /Jarkko

It's easy to say in retroperpective that code is "ugly". I would use
strong consideration before using that adjective for mainline code.
Rarely when you do something new first the form will be polished.

I was too steep with the policy above. It's not exactly like that in the
strict sense. It's always case by case like for any commit. However, it
is good to remember that "ugliness" does not cause regressions.

/Jarkko

RE: [PATCH 3/4] char/tpm: Improve a size determination in nine functions

[Alexander.Steffen]

> On Fri, Oct 20, 2017 at 12:01:39PM +0300, Jarkko Sakkinen wrote:
> > On Thu, Oct 19, 2017 at 04:58:23PM +0000,
> Alexander.Steffen@infineon.com wrote:
> > > > On Tue, Oct 17, 2017 at 11:50:05AM +0000,
> Alexander.Steffen@infineon.com
> > > > wrote:
> > > > > > > Replace the specification of data structures by pointer
> dereferences
> > > > > > > as the parameter for the operator "sizeof" to make the
> corresponding
> > > > > > > size
> > > > > > > determination a bit safer according to the Linux coding style
> > > > > > > convention.
> > > > > >
> > > > > >
> > > > > > This patch does one style in favor of the other.
> > > > >
> > > > > I actually prefer that style, so I'd welcome this change :)
> > > > >
> > > > > > At the end it's Jarkko's call, though I would NAK this as I think some
> > > > > > one already told this to you for some other similar patch(es).
> > > > > >
> > > > > >
> > > > > > I even would suggest to stop doing this noisy stuff, which keeps
> people
> > > > > > busy for nothing.
> > > > >
> > > > > Cleaning up old code is also worth something, even if does not change
> > > > > one bit in the assembly output in the end...
> > > > >
> > > > > Alexander
> > > >
> > > > Quite insignificant clean up it is that does more harm that gives any
> > > > benefit as any new change adds debt to backporting.
> > > >
> > > > Anyway, this has been a useful patch set for me in the sense that I have
> > > > clearer picture now on discarding/accepting commits.
> > >
> > > Indeed. I have now a better understanding for why some code looks as
> > > ugly as it does.
> > >
> > > > One line minor
> > > > clean up will be from now on automatic NAK unless it causes a compiler
> > > > warning or some other visible side-effect.
> > >
> > > Not a nice policy, but at least a policy. I have deleted the tasks
> > > that I had still planned for other cleanup activities.
> > >
> > > Alexander
> >
> > 1/4 and 2/4 are sensible clean ups as long as the commit message is
> > refined.
> >
> > Moving more functions to use tpm_buf instead of nasty tpm_cmd_t are
> > also welcome changes.
> >
> > Documenting functions (exported mainly) is also welcome. Or refining
> > documentation.
> >
> > It's really case by case. The important thing in small clean ups is
> > a clearly written commit message that explains rationale.
> >
> > /Jarkko
> 
> It's easy to say in retroperpective that code is "ugly". I would use
> strong consideration before using that adjective for mainline code.
> Rarely when you do something new first the form will be polished.

(Let's not argue over words, not being a native speaker, I'll probably not choose the perfect expression all the time.)

My assumption is that in many cases the code was not like that from the beginning. Over time, new functionality got added, interfaces expanded, etc. But when the goal tends to be to keep the changes minimal, then it is only natural for everyone to be focused on their small parts of the code, and not clean up the surrounding areas (or better integrate with them).

> I was too steep with the policy above. It's not exactly like that in the
> strict sense. It's always case by case like for any commit. However, it
> is good to remember that "ugliness" does not cause regressions.

Not by itself, no. But it causes cognitive strain while working with the code, and that might lead to bugs.

Alexander

Re: [PATCH 3/4] char/tpm: Improve a size determination in nine functions

[Dan Carpenter]

On Thu, Oct 19, 2017 at 04:58:23PM +0000, Alexander.Steffen@infineon.com wrote:
> > On Tue, Oct 17, 2017 at 11:50:05AM +0000, Alexander.Steffen@infineon.com
> > wrote:
> > > > > Replace the specification of data structures by pointer dereferences
> > > > > as the parameter for the operator "sizeof" to make the corresponding
> > > > > size
> > > > > determination a bit safer according to the Linux coding style
> > > > > convention.
> > > >
> > > >
> > > > This patch does one style in favor of the other.
> > >
> > > I actually prefer that style, so I'd welcome this change :)
> > >
> > > > At the end it's Jarkko's call, though I would NAK this as I think some
> > > > one already told this to you for some other similar patch(es).
> > > >
> > > >
> > > > I even would suggest to stop doing this noisy stuff, which keeps people
> > > > busy for nothing.
> > >
> > > Cleaning up old code is also worth something, even if does not change
> > > one bit in the assembly output in the end...
> > >
> > > Alexander
> > 
> > Quite insignificant clean up it is that does more harm that gives any
> > benefit as any new change adds debt to backporting.
> > 
> > Anyway, this has been a useful patch set for me in the sense that I have
> > clearer picture now on discarding/accepting commits.
> 
> Indeed. I have now a better understanding for why some code looks as ugly as it does.
> 

These patches aren't a part of a sensible attempt to clean up the code.

The first two patches are easy to review and have a clear benefit so
that's fine any time.

Patch 3 updates the code to a new style guideline but it doesn't really
improve readability that much.  Those sorts of patches are hard to
review because you have to verify that the object code doesn't change.
Plus it messes up git blame.  It's good for new code and staging, but
for old code, it's probably only worth it if there are other patches
which make worth the price.  You're basically applying it to make the
patch sender happy.

With patch 4, the tpm_ibmvtpm_probe() function was buggy and it's still
buggy after the patch is applied.  It's a waste of time re-arranging the
code if you aren't going to fix the bugs.

regards,
dan carpenter