Re: [net-next PATCH v3 3/3] net: phy: add support for PHY package MMD read/write

Re: [net-next PATCH v3 3/3] net: phy: add support for PHY package MMD read/write

[Jakub Kicinski]

On Tue, 5 Dec 2023 15:10:50 +0000 Russell King (Oracle) wrote:
> I've raised this before in other subsystems, and it's suggested that
> it's better to have it in the .c file. I guess the reason is that it's
> more obvious that the function is documented when modifying it, so
> there's a higher probability that the kdoc will get updated when the
> function is altered.

Plus I think people using IDEs (i.e. not me) may use the "jump to
definition" functionality, to find the doc? 

TBH I thought putting kdoc in the C source was documented in the coding
style, but I can't find any mention of it now.

Re: [net-next PATCH v3 3/3] net: phy: add support for PHY package MMD read/write

[Russell King (Oracle)]

On Tue, Dec 05, 2023 at 07:29:12AM -0800, Jakub Kicinski wrote:
> On Tue, 5 Dec 2023 15:10:50 +0000 Russell King (Oracle) wrote:
> > I've raised this before in other subsystems, and it's suggested that
> > it's better to have it in the .c file. I guess the reason is that it's
> > more obvious that the function is documented when modifying it, so
> > there's a higher probability that the kdoc will get updated when the
> > function is altered.
> 
> Plus I think people using IDEs (i.e. not me) may use the "jump to
> definition" functionality, to find the doc? 
> 
> TBH I thought putting kdoc in the C source was documented in the coding
> style, but I can't find any mention of it now.

Well, in Documentation/doc-guide/kernel-doc.rst:

  The function and type kernel-doc comments should be placed just before
  the function or type being described in order to maximise the chance
  that somebody changing the code will also change the documentation.

That implies (but not explicitly) that it should be at the function
definition site, since "changing the code" is used as an argument as
I did in my previous email.

Secondly, this document goes on to give an example of running
scripts/kernel-doc on a .c file.

Thirdly, there are seven references in this document of kernel-doc
in .c files, and only one for kernel-doc in a .h file. So this suggests
that "it will be in a .c file" isn't a rule (it can't be because of
documenting structures!)

So let's not get hung up on whether it should be in .c or .h because I
think that isn't relevant. Instead, I think it's about "it should be at
the definition site" - that being a structure definition or a function
definition, and not at a function prototype.

The only exception I can think of is the style I've used in
linux/phylink.h for the _method_ definitions which look like function
prototypes - that's just a work-around because one can't kernel-doc
the structure-of-function-pointers and document the function parameters
without jumping through that hoop, and it would be silly to document
the methods in some random driver!

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [net-next PATCH v3 3/3] net: phy: add support for PHY package MMD read/write

[Jeff Johnson]

On 12/5/2023 8:11 AM, Russell King (Oracle) wrote:
> On Tue, Dec 05, 2023 at 07:29:12AM -0800, Jakub Kicinski wrote:
>> On Tue, 5 Dec 2023 15:10:50 +0000 Russell King (Oracle) wrote:
>>> I've raised this before in other subsystems, and it's suggested that
>>> it's better to have it in the .c file. I guess the reason is that it's
>>> more obvious that the function is documented when modifying it, so
>>> there's a higher probability that the kdoc will get updated when the
>>> function is altered.
>>
>> Plus I think people using IDEs (i.e. not me) may use the "jump to
>> definition" functionality, to find the doc? 
>>
>> TBH I thought putting kdoc in the C source was documented in the coding
>> style, but I can't find any mention of it now.
> 
> Well, in Documentation/doc-guide/kernel-doc.rst:
> 
>   The function and type kernel-doc comments should be placed just before
>   the function or type being described in order to maximise the chance
>   that somebody changing the code will also change the documentation.
> 
> That implies (but not explicitly) that it should be at the function
> definition site, since "changing the code" is used as an argument as
> I did in my previous email.
> 
> Secondly, this document goes on to give an example of running
> scripts/kernel-doc on a .c file.
> 
> Thirdly, there are seven references in this document of kernel-doc
> in .c files, and only one for kernel-doc in a .h file. So this suggests
> that "it will be in a .c file" isn't a rule (it can't be because of
> documenting structures!)
> 
> So let's not get hung up on whether it should be in .c or .h because I
> think that isn't relevant. Instead, I think it's about "it should be at
> the definition site" - that being a structure definition or a function
> definition, and not at a function prototype.
> 
> The only exception I can think of is the style I've used in
> linux/phylink.h for the _method_ definitions which look like function
> prototypes - that's just a work-around because one can't kernel-doc
> the structure-of-function-pointers and document the function parameters
> without jumping through that hoop, and it would be silly to document
> the methods in some random driver!
> 

The Linux Kernel philosophy of documenting functions instead of
prototypes has always bothered me since I'm "old school" and am
ingrained with the software engineering philosophy that you document
interfaces, not implementations. This was reinforced early in my career
by working on multiple projects in different programming languages using
processes outlined in DOD-STD-2167A, and for some projects, especially
ones written in Ada, the header files were the design and the documentation.

This philosophy was further enforced when working with closed source
projects (Windows, IOS, VxWorks) where all the documentation was
contained in shared header files.

So in my experience a function prototype IS the function definition, and
the actual function is just the implementation of that definition.

But that thinking obviously isn't shared by others.

/jeff

Re: [net-next PATCH v3 3/3] net: phy: add support for PHY package MMD read/write

[Russell King (Oracle)]

On Tue, Dec 05, 2023 at 09:44:05AM -0800, Jeff Johnson wrote:
> On 12/5/2023 8:11 AM, Russell King (Oracle) wrote:
> > On Tue, Dec 05, 2023 at 07:29:12AM -0800, Jakub Kicinski wrote:
> >> On Tue, 5 Dec 2023 15:10:50 +0000 Russell King (Oracle) wrote:
> >>> I've raised this before in other subsystems, and it's suggested that
> >>> it's better to have it in the .c file. I guess the reason is that it's
> >>> more obvious that the function is documented when modifying it, so
> >>> there's a higher probability that the kdoc will get updated when the
> >>> function is altered.
> >>
> >> Plus I think people using IDEs (i.e. not me) may use the "jump to
> >> definition" functionality, to find the doc? 
> >>
> >> TBH I thought putting kdoc in the C source was documented in the coding
> >> style, but I can't find any mention of it now.
> > 
> > Well, in Documentation/doc-guide/kernel-doc.rst:
> > 
> >   The function and type kernel-doc comments should be placed just before
> >   the function or type being described in order to maximise the chance
> >   that somebody changing the code will also change the documentation.
> > 
> > That implies (but not explicitly) that it should be at the function
> > definition site, since "changing the code" is used as an argument as
> > I did in my previous email.
> > 
> > Secondly, this document goes on to give an example of running
> > scripts/kernel-doc on a .c file.
> > 
> > Thirdly, there are seven references in this document of kernel-doc
> > in .c files, and only one for kernel-doc in a .h file. So this suggests
> > that "it will be in a .c file" isn't a rule (it can't be because of
> > documenting structures!)
> > 
> > So let's not get hung up on whether it should be in .c or .h because I
> > think that isn't relevant. Instead, I think it's about "it should be at
> > the definition site" - that being a structure definition or a function
> > definition, and not at a function prototype.
> > 
> > The only exception I can think of is the style I've used in
> > linux/phylink.h for the _method_ definitions which look like function
> > prototypes - that's just a work-around because one can't kernel-doc
> > the structure-of-function-pointers and document the function parameters
> > without jumping through that hoop, and it would be silly to document
> > the methods in some random driver!
> > 
> 
> The Linux Kernel philosophy of documenting functions instead of
> prototypes has always bothered me since I'm "old school" and am
> ingrained with the software engineering philosophy that you document
> interfaces, not implementations. This was reinforced early in my career
> by working on multiple projects in different programming languages using
> processes outlined in DOD-STD-2167A, and for some projects, especially
> ones written in Ada, the header files were the design and the documentation.
> 
> This philosophy was further enforced when working with closed source
> projects (Windows, IOS, VxWorks) where all the documentation was
> contained in shared header files.
> 
> So in my experience a function prototype IS the function definition, and
> the actual function is just the implementation of that definition.
> 
> But that thinking obviously isn't shared by others.

Interestingly, the view that a function prototype is a function
definition does not seem to be shared by w3school, Microsoft, IBM,
and many more.

If we look at the C99 standard, then 6.9.1 Function definitions gives
the syntax as including a compound-statement, which is defined as
requiring the curley braces and contents. Therefore, a function
definition as defined by the C standard includes its body.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [net-next PATCH v3 3/3] net: phy: add support for PHY package MMD read/write

[Jeff Johnson]

On 12/5/2023 10:14 AM, Russell King (Oracle) wrote:
> On Tue, Dec 05, 2023 at 09:44:05AM -0800, Jeff Johnson wrote:
>> So in my experience a function prototype IS the function definition, and
>> the actual function is just the implementation of that definition.
>>
>> But that thinking obviously isn't shared by others.
> 
> Interestingly, the view that a function prototype is a function
> definition does not seem to be shared by w3school, Microsoft, IBM,
> and many more.
> 
> If we look at the C99 standard, then 6.9.1 Function definitions gives
> the syntax as including a compound-statement, which is defined as
> requiring the curley braces and contents. Therefore, a function
> definition as defined by the C standard includes its body.
> 

Note I was speaking in terms of functional languages in general, not C
specifically. Perhaps I should have used the term "specification"
instead of "definition" (which would align with the Ada terminology).

Having worked with closed-source systems, especially VxWorks, for many
years (where the header files contain all the documentation), it just
seems strange to embed the documentation in the .c files.

/jeff

Re: [net-next PATCH v3 3/3] net: phy: add support for PHY package MMD read/write

[Andrew Lunn]

> Having worked with closed-source systems, especially VxWorks, for many
> years (where the header files contain all the documentation), it just
> seems strange to embed the documentation in the .c files.

The key words here might be closed-source. With such black boxes, you
don't have access the sources. You cannot look at the source to
understand how a function works. In the open source world, the
comments partially function as an introduction to reading the code and
understanding what it does. You are also encouraged to change the code
if needed, which in the closed source world you cannot do.

Given this discussion, i now think putting the documentation in the .c
file makes more sense. For the generated documentation it does not
matter, but for the reader of the code, having it in the .c files does
seem to make sense.

     Andrew