| Message ID | 83a166af-aecc-649d-dfe3-a72245345209@users.sourceforge.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; >
> > 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
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
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 >
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.
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
> 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
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
>> 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
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
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)?
>> 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
>> 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
> 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
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
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
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
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
>> 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
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
> 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
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
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.
> 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
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
> 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
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?
>> 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
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
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
> 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
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
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
> 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
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
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;