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

[Dan Carpenter]

On Thu, Oct 19, 2017 at 04:58:23PM +, alexander.stef...@infineon.com wrote:
> > On Tue, Oct 17, 2017 at 11:50:05AM +, alexander.stef...@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

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 +,
> alexander.stef...@infineon.com wrote:
> > > > On Tue, Oct 17, 2017 at 11:50:05AM +,
> alexander.stef...@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

[Jarkko Sakkinen]

On Fri, Oct 20, 2017 at 12:01:39PM +0300, Jarkko Sakkinen wrote:
> On Thu, Oct 19, 2017 at 04:58:23PM +, alexander.stef...@infineon.com 
> wrote:
> > > On Tue, Oct 17, 2017 at 11:50:05AM +, alexander.stef...@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

[Jarkko Sakkinen]

On Thu, Oct 19, 2017 at 04:58:23PM +, alexander.stef...@infineon.com wrote:
> > On Tue, Oct 17, 2017 at 11:50:05AM +, alexander.stef...@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

[Alexander.Steffen]

> On Tue, Oct 17, 2017 at 11:50:05AM +, alexander.stef...@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: [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 +, alexander.stef...@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: [PATCH 3/4] char/tpm: Improve a size determination in nine functions

[Jarkko Sakkinen]

On Tue, Oct 17, 2017 at 11:50:05AM +, alexander.stef...@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 02:03:02PM +0300, Andy Shevchenko wrote:
> On Mon, 2017-10-16 at 19:33 +0200, SF Markus Elfring wrote:
> > From: Markus Elfring 
> > 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

[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 +, alexander.stef...@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: [PATCH 3/4] char/tpm: Improve a size determination in nine functions

[Alexander.Steffen]

> On Tue, 2017-10-17 at 11:50 +, alexander.stef...@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

[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 3/4] char/tpm: Improve a size determination in nine functions

[Julia Lawall]

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 +, alexander.stef...@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 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 +, alexander.stef...@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

[Andy Shevchenko]

On Tue, 2017-10-17 at 08:52 -0400, Mimi Zohar wrote:
> On Tue, 2017-10-17 at 11:50 +, alexander.stef...@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 
Intel Finland Oy

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

[Julia Lawall]

On Tue, 17 Oct 2017, Mimi Zohar wrote:

> On Tue, 2017-10-17 at 11:50 +, alexander.stef...@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 majord...@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

[Mimi Zohar]

On Tue, 2017-10-17 at 11:50 +, alexander.stef...@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

[Andy Shevchenko]

On Mon, 2017-10-16 at 19:33 +0200, SF Markus Elfring wrote:
> From: Markus Elfring 
> 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 
> ---
>  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(>dev, sizeof(struct
> st33zp24_i2c_phy),
> -GFP_KERNEL);
> + phy = devm_kzalloc(>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, sizeof(struct
> st33zp24_spi_phy),
> -GFP_KERNEL);
> + phy = devm_kzalloc(>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 

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

[SF Markus Elfring]

From: Markus Elfring 
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 
---
 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(>dev, sizeof(struct st33zp24_i2c_phy),
-  GFP_KERNEL);
+   phy = devm_kzalloc(>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, sizeof(struct st33zp24_spi_phy),
-  GFP_KERNEL);
+   phy = devm_kzalloc(>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,