| 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
| ||
|
Date: Thu, 11 Feb 2021 00:55:31 +0100
From: Michael Walle <michael@...le.cc>
To: netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Cc: Andrew Lunn <andrew@...n.ch>,
Heiner Kallweit <hkallweit1@...il.com>,
Russell King <linux@...linux.org.uk>,
"David S . Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH net-next v3 7/9] net: phy: icplus: fix paged register
access
Am 2021-02-10 22:08, schrieb Michael Walle:
> Registers >= 16 are paged. Be sure to set the page. It seems this was
> working for now, because the default is correct for the registers used
> in the driver at the moment. But this will also assume, nobody will
> change the page select register before linux is started. The page
> select
> register is _not_ reset with a soft reset of the PHY.
>
> To ease the function reuse between the non-paged register space of the
> IP101A and the IP101G, add noop read_page()/write_page() callbacks so
> the IP101G functions can also be used for the IP101A.
>
> Signed-off-by: Michael Walle <michael@...le.cc>
> ---
> Changes since v2:
> - none
>
> Changes since v1:
> - introduce a noop read/write_page() for the IP101A
> - also use phy_*_paged() for the interrupt status register
>
> Andrew, I've dropped your Reviewed-by because of this.
>
> drivers/net/phy/icplus.c | 65 ++++++++++++++++++++++++++++++++--------
> 1 file changed, 52 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/net/phy/icplus.c b/drivers/net/phy/icplus.c
> index bc2b58061507..7e0ef05b1cae 100644
> --- a/drivers/net/phy/icplus.c
> +++ b/drivers/net/phy/icplus.c
> @@ -49,6 +49,8 @@ MODULE_LICENSE("GPL");
> #define IP101G_DIGITAL_IO_SPEC_CTRL 0x1d
> #define IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32 BIT(2)
>
> +#define IP101G_DEFAULT_PAGE 16
> +
> #define IP175C_PHY_ID 0x02430d80
> #define IP1001_PHY_ID 0x02430d90
> #define IP101A_PHY_ID 0x02430c54
> @@ -211,23 +213,25 @@ static int ip101a_g_probe(struct phy_device
> *phydev)
> static int ip101a_g_config_intr_pin(struct phy_device *phydev)
> {
> struct ip101a_g_phy_priv *priv = phydev->priv;
> - int err;
> + int oldpage, err;
besides this being uninitialized
> +
> + oldpage = phy_select_page(phydev, IP101G_DEFAULT_PAGE);
this is also missing a check for negative return values
will be fixed in v4
-michael
>
> /* configure the RXER/INTR_32 pin of the 32-pin IP101GR if needed: */
> switch (priv->sel_intr32) {
> case IP101GR_SEL_INTR32_RXER:
> - err = phy_modify(phydev, IP101G_DIGITAL_IO_SPEC_CTRL,
> - IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32, 0);
> + err = __phy_modify(phydev, IP101G_DIGITAL_IO_SPEC_CTRL,
> + IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32, 0);
> if (err < 0)
> - return err;
> + goto out;
> break;
>
> case IP101GR_SEL_INTR32_INTR:
> - err = phy_modify(phydev, IP101G_DIGITAL_IO_SPEC_CTRL,
> - IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32,
> - IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32);
> + err = __phy_modify(phydev, IP101G_DIGITAL_IO_SPEC_CTRL,
> + IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32,
> + IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32);
> if (err < 0)
> - return err;
> + goto out;
> break;
>
> default:
> @@ -241,7 +245,8 @@ static int ip101a_g_config_intr_pin(struct
> phy_device *phydev)
> break;
> }
>
> - return 0;
> +out:
> + return phy_restore_page(phydev, oldpage, err);
> }
>
> static int ip101a_config_init(struct phy_device *phydev)
> @@ -263,8 +268,10 @@ static int ip101g_config_init(struct phy_device
> *phydev)
>
> static int ip101a_g_ack_interrupt(struct phy_device *phydev)
> {
> - int err = phy_read(phydev, IP101A_G_IRQ_CONF_STATUS);
> + int err;
>
> + err = phy_read_paged(phydev, IP101G_DEFAULT_PAGE,
> + IP101A_G_IRQ_CONF_STATUS);
> if (err < 0)
> return err;
>
> @@ -283,10 +290,12 @@ static int ip101a_g_config_intr(struct phy_device
> *phydev)
>
> /* INTR pin used: Speed/link/duplex will cause an interrupt */
> val = IP101A_G_IRQ_PIN_USED;
> - err = phy_write(phydev, IP101A_G_IRQ_CONF_STATUS, val);
> + err = phy_write_paged(phydev, IP101G_DEFAULT_PAGE,
> + IP101A_G_IRQ_CONF_STATUS, val);
> } else {
> val = IP101A_G_IRQ_ALL_MASK;
> - err = phy_write(phydev, IP101A_G_IRQ_CONF_STATUS, val);
> + err = phy_write_paged(phydev, IP101G_DEFAULT_PAGE,
> + IP101A_G_IRQ_CONF_STATUS, val);
> if (err)
> return err;
>
> @@ -300,7 +309,8 @@ static irqreturn_t
> ip101a_g_handle_interrupt(struct phy_device *phydev)
> {
> int irq_status;
>
> - irq_status = phy_read(phydev, IP101A_G_IRQ_CONF_STATUS);
> + irq_status = phy_read_paged(phydev, IP101G_DEFAULT_PAGE,
> + IP101A_G_IRQ_CONF_STATUS);
> if (irq_status < 0) {
> phy_error(phydev);
> return IRQ_NONE;
> @@ -316,6 +326,31 @@ static irqreturn_t
> ip101a_g_handle_interrupt(struct phy_device *phydev)
> return IRQ_HANDLED;
> }
>
> +/* The IP101A doesn't really have a page register. We just pretend to
> have one
> + * so we can use the paged versions of the callbacks of the IP101G.
> + */
> +static int ip101a_read_page(struct phy_device *phydev)
> +{
> + return IP101G_DEFAULT_PAGE;
> +}
> +
> +static int ip101a_write_page(struct phy_device *phydev, int page)
> +{
> + WARN_ONCE(page != IP101G_DEFAULT_PAGE, "wrong page selected\n");
> +
> + return 0;
> +}
> +
> +static int ip101g_read_page(struct phy_device *phydev)
> +{
> + return __phy_read(phydev, IP101G_PAGE_CONTROL);
> +}
> +
> +static int ip101g_write_page(struct phy_device *phydev, int page)
> +{
> + return __phy_write(phydev, IP101G_PAGE_CONTROL, page);
> +}
> +
> static int ip101a_g_has_page_register(struct phy_device *phydev)
> {
> int oldval, val, ret;
> @@ -390,6 +425,8 @@ static struct phy_driver icplus_driver[] = {
> .name = "ICPlus IP101A",
> .match_phy_device = ip101a_match_phy_device,
> .probe = ip101a_g_probe,
> + .read_page = ip101a_read_page,
> + .write_page = ip101a_write_page,
> .config_intr = ip101a_g_config_intr,
> .handle_interrupt = ip101a_g_handle_interrupt,
> .config_init = ip101a_config_init,
> @@ -400,6 +437,8 @@ static struct phy_driver icplus_driver[] = {
> .name = "ICPlus IP101G",
> .match_phy_device = ip101g_match_phy_device,
> .probe = ip101a_g_probe,
> + .read_page = ip101g_read_page,
> + .write_page = ip101g_write_page,
> .config_intr = ip101a_g_config_intr,
> .handle_interrupt = ip101a_g_handle_interrupt,
> .config_init = ip101g_config_init,
--
-michael
Powered by blists - more mailing lists