| 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
| ||
|
Message-ID: <Y++BXkdXO8oysQ8M@shell.armlinux.org.uk>
Date: Fri, 17 Feb 2023 13:30:06 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Horatiu Vultur <horatiu.vultur@...rochip.com>
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
andrew@...n.ch, hkallweit1@...il.com, davem@...emloft.net,
edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
richardcochran@...il.com
Subject: Re: [PATCH net-next] net: phy: micrel: Add support for PTP_PF_PEROUT
for lan8841
On Fri, Feb 17, 2023 at 08:52:13AM +0100, Horatiu Vultur wrote:
> +static void lan8841_ptp_perout_off(struct kszphy_ptp_priv *ptp_priv, int pin)
> +{
> + struct phy_device *phydev = ptp_priv->phydev;
> + u16 tmp;
> +
> + tmp = phy_read_mmd(phydev, 2, LAN8841_GPIO_EN) & LAN8841_PTP_GPIO_MASK;
> + tmp &= ~BIT(pin);
> + phy_write_mmd(phydev, 2, LAN8841_GPIO_EN, tmp);
Problem 1: doesn't check the return value of phy_read_mmd(), so a
spurious error results in an error code written back to the register.
Issue 2: please use phy_modify_mmd() and definitions for the MMD. It
probably also makes sense to cache the mask. Thus, this whole thing
becomes:
u16 mask = ~(LAN8841_PTP_GPIO_MASK | BIT(pin));
phy_modify_mmd(phydev, MDIO_MMD_WIS, LAN8841_GPIO_EN, mask, 0);
phy_modify_mmd(phydev, MDIO_MMD_WIS, LAN8841_GPIO_DIR, mask, 0);
phy_modify_mmd(phydev, MDIO_MMD_WIS, LAN8841_GPIO_BUF, mask, 0);
although I'm not sure why you need to mask off bits 15:11.
> +
> + tmp = phy_read_mmd(phydev, 2, LAN8841_GPIO_DIR) & LAN8841_PTP_GPIO_MASK;
> + tmp &= ~BIT(pin);
> + phy_write_mmd(phydev, 2, LAN8841_GPIO_DIR, tmp);
> +
> + tmp = phy_read_mmd(phydev, 2, LAN8841_GPIO_BUF) & LAN8841_PTP_GPIO_MASK;
> + tmp &= ~BIT(pin);
> + phy_write_mmd(phydev, 2, LAN8841_GPIO_BUF, tmp);
> +}
> +
> +static void lan8841_ptp_perout_on(struct kszphy_ptp_priv *ptp_priv, int pin)
> +{
> + struct phy_device *phydev = ptp_priv->phydev;
> + u16 tmp;
> +
> + tmp = phy_read_mmd(phydev, 2, LAN8841_GPIO_EN) & LAN8841_PTP_GPIO_MASK;
> + tmp |= BIT(pin);
> + phy_write_mmd(phydev, 2, LAN8841_GPIO_EN, tmp);
> +
> + tmp = phy_read_mmd(phydev, 2, LAN8841_GPIO_DIR) & LAN8841_PTP_GPIO_MASK;
> + tmp |= BIT(pin);
> + phy_write_mmd(phydev, 2, LAN8841_GPIO_DIR, tmp);
> +
> + tmp = phy_read_mmd(phydev, 2, LAN8841_GPIO_BUF) & LAN8841_PTP_GPIO_MASK;
> + tmp |= BIT(pin);
> + phy_write_mmd(phydev, 2, LAN8841_GPIO_BUF, tmp);
Similar as above.
> +static void lan8841_ptp_remove_event(struct kszphy_ptp_priv *ptp_priv, int pin,
> + u8 event)
> +{
> + struct phy_device *phydev = ptp_priv->phydev;
> + u8 offset;
> + u16 tmp;
> +
> + /* Not remove pin from the event. GPIO_DATA_SEL1 contains the GPIO
> + * pins 0-4 while GPIO_DATA_SEL2 contains GPIO pins 5-9, therefore
> + * depending on the pin, it requires to read a different register
> + */
> + if (pin < 5) {
> + tmp = phy_read_mmd(phydev, 2, LAN8841_GPIO_DATA_SEL1);
> + offset = pin;
> + } else {
> + tmp = phy_read_mmd(phydev, 2, LAN8841_GPIO_DATA_SEL2);
> + offset = pin - 5;
> + }
> + tmp &= ~(LAN8841_GPIO_DATA_SEL_GPIO_DATA_SEL_EVENT_MASK << (3 * offset));
> + if (pin < 5)
> + phy_write_mmd(phydev, 2, LAN8841_GPIO_DATA_SEL1, tmp);
> + else
> + phy_write_mmd(phydev, 2, LAN8841_GPIO_DATA_SEL2, tmp);
This could be much simpler using phy_modify_mmd().
> +
> + /* Disable the event */
> + tmp = phy_read_mmd(phydev, 2, LAN8841_PTP_GENERAL_CONFIG);
> + if (event == LAN8841_EVENT_A) {
> + tmp &= ~LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_POL_A;
> + tmp &= ~LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_A_MASK;
> + } else {
> + tmp &= ~LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_POL_A;
> + tmp &= ~LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_A_MASK;
> + }
> + phy_write_mmd(phydev, 2, LAN8841_PTP_GENERAL_CONFIG, tmp);
Ditto... and the theme seems to continue throughout the rest of this
patch.
> +static int lan8841_ptp_perout(struct ptp_clock_info *ptp,
> + struct ptp_clock_request *rq, int on)
> +{
> + struct kszphy_ptp_priv *ptp_priv = container_of(ptp, struct kszphy_ptp_priv,
> + ptp_clock_info);
> + struct phy_device *phydev = ptp_priv->phydev;
> + struct timespec64 ts_on, ts_period;
> + s64 on_nsec, period_nsec;
> + int pulse_width;
> + int pin;
> +
> + if (rq->perout.flags & ~PTP_PEROUT_DUTY_CYCLE)
> + return -EOPNOTSUPP;
> +
> + pin = ptp_find_pin(ptp_priv->ptp_clock, PTP_PF_PEROUT, rq->perout.index);
> + if (pin == -1 || pin >= LAN8841_PTP_GPIO_NUM)
> + return -EINVAL;
> +
> + if (!on) {
> + lan8841_ptp_perout_off(ptp_priv, pin);
> + lan8841_ptp_remove_event(ptp_priv, LAN8841_EVENT_A, pin);
> + return 0;
> + }
> +
> + ts_on.tv_sec = rq->perout.on.sec;
> + ts_on.tv_nsec = rq->perout.on.nsec;
> + on_nsec = timespec64_to_ns(&ts_on);
> +
> + ts_period.tv_sec = rq->perout.period.sec;
> + ts_period.tv_nsec = rq->perout.period.nsec;
> + period_nsec = timespec64_to_ns(&ts_period);
> +
> + if (period_nsec < 200) {
> + phydev_warn(phydev,
> + "perout period too small, minimum is 200 nsec\n");
I'm not sure using the kernel log to print such things is a good idea,
especially without rate limiting.
> @@ -3874,7 +4220,24 @@ static int lan8841_probe(struct phy_device *phydev)
> priv = phydev->priv;
> ptp_priv = &priv->ptp_priv;
>
> + ptp_priv->pin_config = devm_kmalloc_array(&phydev->mdio.dev,
> + LAN8841_PTP_GPIO_NUM,
> + sizeof(*ptp_priv->pin_config),
> + GFP_KERNEL);
devm_kcalloc() to avoid the memset() below?
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Powered by blists - more mailing lists