netdev - Re: [PATCH v3] drivers/net/phy: add driver for Microchip LAN867x 10BASE-T1S PHY

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <ZEE5XwqYrZfyhNXT@debian>
Date:   Thu, 20 Apr 2023 15:08:47 +0200
From:   Ramón Nordin Rodriguez 
        <ramon.nordin.rodriguez@...roamp.se>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org,
        Vladimir Oltean <olteanv@...il.com>
Subject: Re: [PATCH v3] drivers/net/phy: add driver for Microchip LAN867x
 10BASE-T1S PHY

On Thu, Apr 20, 2023 at 02:43:45PM +0200, Andrew Lunn wrote:
> On Wed, Apr 19, 2023 at 06:16:05PM +0200, Ramón Nordin Rodriguez wrote:
> > Changes:
> >     v2:
> > - Removed mentioning of not supporting auto-negotiation from commit
> >   message
> > - Renamed file drivers/net/phy/lan867x.c ->
> >   drivers/net/phy/microchip_t1s.c
> > - Renamed Kconfig option to reflect implementation filename (from
> >   LAN867X_PHY to MICROCHIP_T1S_PHY)
> > - Moved entry in drivers/net/phy/KConfig to correct sort order
> > - Moved entry in drivers/net/phy/Makefile to correct sort order
> > - Moved variable declarations to conform to reverse christmas tree order
> >   (in func lan867x_config_init)
> > - Moved register write to disable chip interrupts to func lan867x_config_init, when omitting the irq disable all togheter I got null pointer dereference, see the call trace below:
> > 
> >     Call Trace:
> >      <TASK>
> >      phy_interrupt+0xa8/0xf0 [libphy]
> >      irq_thread_fn+0x1c/0x60
> >      irq_thread+0xf7/0x1c0
> >      ? __pfx_irq_thread_dtor+0x10/0x10
> >      ? __pfx_irq_thread+0x10/0x10
> >      kthread+0xe6/0x110
> >      ? __pfx_kthread+0x10/0x10
> >      ret_from_fork+0x29/0x50
> >      </TASK>
> > 
> > - Removed func lan867x_config_interrupt and removed the member
> >   .config_intr from the phy_driver struct
> > 
> >     v3:
> > - Indentation level in drivers/net/phy/Kconfig
> > - Moved const arrays into global scope and made them static in order to have
> >   them placed in the .rodata section
> > - Renamed array variables, since they are no longer as closely scoped as
> >   earlier
> > - Added comment about why phy_write_mmd is used over phy_modify_mmd
> >   (this should have been addressed in the V2 change since it was brought
> >   up in the V1 review)
> > - Return result of last call instead of saving it in a var and then
> >   returning the var (in lan867x_config_init)
> > 
> > Testing:
> > This has been tested with ethtool --set/get-plca-cfg and verified on an
> > oscilloscope where it was observed that:
> > - The PLCA beacon was enabled/disabled when setting the node-id to 0/not
> >   0
> > - The PLCA beacon is transmitted with the expected frequency when
> >   changing max nodes
> > - Two devices using the evaluation board EVB-LAN8670-USB could ping each
> >   other
> > 
> > 
> > This patch adds support for the Microchip LAN867x 10BASE-T1S family
> > (LAN8670/1/2). The driver supports P2MP with PLCA.
> > 
> > Signed-off-by: Ramón Nordin Rodriguez <ramon.nordin.rodriguez@...roamp.se>
> 
> All the above ends up in the commit message, as you see in git log. So
> this last paragraph should really be first. As you have it, the
> history of the patch will also be included in the commit. Most Linux
> subsystems don't want that, although DaveM has argued it maybe should
> be in the commit message. But i personally think we have good archives
> of emails, and search engines for those archives, so i don't see the
> need.
> 

The intention was not to include the history in the commit message, as
you allude to this was a misstake on my part.

> Anything you put after the --- will get discarded when the Maintainers
> perform the merge. So i suggest you move most of what you have in the
> commit message below the ---. You can also have two ---, which is how
> i tend to do it, so i can keep the history in my git repo, but it then
> gets removed when merged upstream.

I'll move the history to after the ---

> 
> Additionally, you should add any Reviewed-by:, Acked-by: to your
> patches. Only discard them if you make major changed which invalidates
> any reviews.
> 
>     Andrew

I will post a V4 soon as the 24h window closes on V4, with
- history after the patch
- reviewers:
    - Andrew Lunn <andrew@...n.ch>
    - Vladimir Oltean <olteanv@...il.com>

Hoping this is the last of the newbie misstakes I've managed to pull
off.
Ramón