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 for Android: free password hash cracker in your pocket

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

Message-ID: <b93039ef-a593-4acd-b9c1-3f3e6b79497d@lunn.ch>
Date:   Thu, 20 Apr 2023 14:43:45 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Ramón Nordin Rodriguez 
        <ramon.nordin.rodriguez@...roamp.se>
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 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.

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.

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