| 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: Wed, 30 Dec 2020 08:39:34 +0100
From: Heiner Kallweit <hkallweit1@...il.com>
To: DENG Qingfang <dqfext@...il.com>,
"David S. Miller" <davem@...emloft.net>,
Andrew Lunn <andrew@...n.ch>,
Florian Fainelli <f.fainelli@...il.com>,
Jakub Kicinski <kuba@...nel.org>,
Landen Chao <Landen.Chao@...iatek.com>,
Marc Zyngier <maz@...nel.org>,
Matthias Brugger <matthias.bgg@...il.com>,
Philipp Zabel <p.zabel@...gutronix.de>,
Russell King <linux@...linux.org.uk>,
Sean Wang <sean.wang@...iatek.com>,
Thomas Gleixner <tglx@...utronix.de>,
Vivien Didelot <vivien.didelot@...il.com>,
Vladimir Oltean <olteanv@...il.com>,
linux-kernel@...r.kernel.org, netdev@...r.kernel.org
Cc: Weijie Gao <weijie.gao@...iatek.com>,
Chuanhong Guo <gch981213@...il.com>,
Linus Walleij <linus.walleij@...aro.org>,
René van Dorst <opensource@...rst.com>
Subject: Re: Registering IRQ for MT7530 internal PHYs
On 30.12.2020 05:22, DENG Qingfang wrote:
> Hi,
>
> I added MT7530 IRQ support and registered its internal PHYs to IRQ.
> It works but my patch used two hacks.
>
> 1. Removed phy_drv_supports_irq check, because config_intr and
> handle_interrupt are not set for Generic PHY.
>
I don't think that's the best option. First, did you check which
consequences this has? The check is there for a reason.
You may want to add a PHY driver for your chip. Supposedly it
supports at least PHY suspend/resume. You can use the RTL8366RB
PHY driver as template.
> 2. Allocated ds->slave_mii_bus before calling ds->ops->setup, because
> we cannot call dsa_slave_mii_bus_init which is private.
>
> Any better ideas?
>
> Regards,
> Qingfang
>
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index a67cac15a724..d59a8c50ede3 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -10,6 +10,7 @@
> #include <linux/mfd/syscon.h>
> #include <linux/module.h>
> #include <linux/netdevice.h>
> +#include <linux/of_irq.h>
> #include <linux/of_mdio.h>
> #include <linux/of_net.h>
> #include <linux/of_platform.h>
> @@ -1639,6 +1640,125 @@ mtk_get_tag_protocol(struct dsa_switch *ds, int port,
> }
> }
>
> +static irqreturn_t
> +mt7530_irq(int irq, void *data)
> +{
> + struct mt7530_priv *priv = data;
> + bool handled = false;
> + int phy;
> + u32 val;
> +
> + val = mt7530_read(priv, MT7530_SYS_INT_STS);
> + mt7530_write(priv, MT7530_SYS_INT_STS, val);
> +
> + dev_info_ratelimited(priv->dev, "interrupt status: 0x%08x\n", val);
> + dev_info_ratelimited(priv->dev, "interrupt enable: 0x%08x\n", mt7530_read(priv, MT7530_SYS_INT_EN));
> +
This is debug code to be removed in the final version?
> + for (phy = 0; phy < MT7530_NUM_PHYS; phy++) {
> + if (val & BIT(phy)) {
> + unsigned int child_irq;
> +
> + child_irq = irq_find_mapping(priv->irq_domain, phy);
> + handle_nested_irq(child_irq);
> + handled = true;
> + }
> + }
> +
> + return handled ? IRQ_HANDLED : IRQ_NONE;
IRQ_RETVAL() could be used here.
> +}
> +
> +static void mt7530_irq_mask(struct irq_data *d)
> +{
> + struct mt7530_priv *priv = irq_data_get_irq_chip_data(d);
> +
> + priv->irq_enable &= ~BIT(d->hwirq);
Here you don't actually do something. HW doesn't support masking
interrupt generation for a port?
> +}
> +
> +static void mt7530_irq_unmask(struct irq_data *d)
> +{
> + struct mt7530_priv *priv = irq_data_get_irq_chip_data(d);
> +
> + priv->irq_enable |= BIT(d->hwirq);
> +}
> +
> +static void mt7530_irq_bus_lock(struct irq_data *d)
> +{
> + struct mt7530_priv *priv = irq_data_get_irq_chip_data(d);
> +
> + mutex_lock(&priv->reg_mutex);
> +}
> +
> +static void mt7530_irq_bus_sync_unlock(struct irq_data *d)
> +{
> + struct mt7530_priv *priv = irq_data_get_irq_chip_data(d);
> +
> + mt7530_write(priv, MT7530_SYS_INT_EN, priv->irq_enable);
> + mutex_unlock(&priv->reg_mutex);
> +}
> +
> +static struct irq_chip mt7530_irq_chip = {
> + .name = MT7530_NAME,
> + .irq_mask = mt7530_irq_mask,
> + .irq_unmask = mt7530_irq_unmask,
> + .irq_bus_lock = mt7530_irq_bus_lock,
> + .irq_bus_sync_unlock = mt7530_irq_bus_sync_unlock,
> +};
> +
> +static int
> +mt7530_irq_map(struct irq_domain *domain, unsigned int irq,
> + irq_hw_number_t hwirq)
> +{
> + irq_set_chip_data(irq, domain->host_data);
> + irq_set_chip_and_handler(irq, &mt7530_irq_chip, handle_simple_irq);
> + irq_set_noprobe(irq);
> +
> + return 0;
> +}
> +
> +static const struct irq_domain_ops mt7530_irq_domain_ops = {
> + .map = mt7530_irq_map,
> + .xlate = irq_domain_xlate_onecell,
> +};
> +
> +static void
> +mt7530_irq_init(struct mt7530_priv *priv)
> +{
> + struct mii_bus *bus = priv->ds->slave_mii_bus;
> + struct device *dev = priv->dev;
> + struct device_node *np = dev->of_node;
> + int parent_irq;
> + int phy, ret;
> +
> + parent_irq = of_irq_get(np, 0);
> + if (parent_irq <= 0) {
> + dev_err(dev, "failed to get parent IRQ: %d\n", parent_irq);
> + return;
> + }
> +
> + mt7530_set(priv, MT7530_TOP_SIG_CTRL, TOP_SIG_CTRL_NORMAL);
> + ret = devm_request_threaded_irq(dev, parent_irq, NULL, mt7530_irq,
> + IRQF_ONESHOT, MT7530_NAME, priv);
> + if (ret) {
> + dev_err(dev, "failed to request IRQ: %d\n", ret);
> + return;
> + }
> +
> + priv->irq_domain = irq_domain_add_linear(np, MT7530_NUM_PHYS,
> + &mt7530_irq_domain_ops, priv);
> + if (!priv->irq_domain) {
> + dev_err(dev, "failed to create IRQ domain\n");
> + return;
> + }
> +
> + /* IRQ for internal PHYs */
> + for (phy = 0; phy < MT7530_NUM_PHYS; phy++) {
> + unsigned int irq = irq_create_mapping(priv->irq_domain, phy);
> +
> + irq_set_parent(irq, parent_irq);
> + bus->irq[phy] = irq;
> + }
> +}
> +
> static int
> mt7530_setup(struct dsa_switch *ds)
> {
> @@ -2578,8 +2698,13 @@ static int
> mt753x_setup(struct dsa_switch *ds)
> {
> struct mt7530_priv *priv = ds->priv;
> + int ret = priv->info->sw_setup(ds);
>
> - return priv->info->sw_setup(ds);
> + /* Setup interrupt */
> + if (!ret)
> + mt7530_irq_init(priv);
> +
> + return ret;
> }
>
> static int
> @@ -2780,6 +2905,9 @@ mt7530_remove(struct mdio_device *mdiodev)
> dev_err(priv->dev, "Failed to disable io pwr: %d\n",
> ret);
>
> + if (priv->irq_domain)
> + irq_domain_remove(priv->irq_domain);
> +
> dsa_unregister_switch(priv->ds);
> mutex_destroy(&priv->reg_mutex);
> }
> @@ -2788,7 +2916,7 @@ static struct mdio_driver mt7530_mdio_driver = {
> .probe = mt7530_probe,
> .remove = mt7530_remove,
> .mdiodrv.driver = {
> - .name = "mt7530",
> + .name = MT7530_NAME,
> .of_match_table = mt7530_of_match,
> },
> };
> diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
> index 32d8969b3ace..b1988d8085bb 100644
> --- a/drivers/net/dsa/mt7530.h
> +++ b/drivers/net/dsa/mt7530.h
> @@ -6,7 +6,10 @@
> #ifndef __MT7530_H
> #define __MT7530_H
>
> +#define MT7530_NAME "mt7530"
> +
> #define MT7530_NUM_PORTS 7
> +#define MT7530_NUM_PHYS 5
> #define MT7530_CPU_PORT 6
> #define MT7530_NUM_FDB_RECORDS 2048
> #define MT7530_ALL_MEMBERS 0xff
> @@ -380,6 +383,12 @@ enum mt7531_sgmii_force_duplex {
> #define SYS_CTRL_SW_RST BIT(1)
> #define SYS_CTRL_REG_RST BIT(0)
>
> +/* Register for system interrupt */
> +#define MT7530_SYS_INT_EN 0x7008
> +
> +/* Register for system interrupt status */
> +#define MT7530_SYS_INT_STS 0x700c
> +
> /* Register for PHY Indirect Access Control */
> #define MT7531_PHY_IAC 0x701C
> #define MT7531_PHY_ACS_ST BIT(31)
> @@ -761,6 +770,8 @@ struct mt7530_priv {
> struct mt7530_port ports[MT7530_NUM_PORTS];
> /* protect among processes for registers access*/
> struct mutex reg_mutex;
> + struct irq_domain *irq_domain;
> + u32 irq_enable;
> };
>
> struct mt7530_hw_vlan_entry {
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 80c2e646c093..cdddc27e2df7 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -2846,12 +2846,6 @@ static int phy_probe(struct device *dev)
>
> phydev->drv = phydrv;
>
> - /* Disable the interrupt if the PHY doesn't support it
> - * but the interrupt is still a valid one
> - */
> - if (!phy_drv_supports_irq(phydrv) && phy_interrupt_is_valid(phydev))
> - phydev->irq = PHY_POLL;
> -
> if (phydrv->flags & PHY_IS_INTERNAL)
> phydev->is_internal = true;
>
> diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
> index 183003e45762..ef5db1106581 100644
> --- a/net/dsa/dsa2.c
> +++ b/net/dsa/dsa2.c
> @@ -448,19 +448,19 @@ static int dsa_switch_setup(struct dsa_switch *ds)
> if (err)
> goto unregister_devlink_ports;
>
> + ds->slave_mii_bus = devm_mdiobus_alloc(ds->dev);
> + if (!ds->slave_mii_bus) {
> + err = -ENOMEM;
> + goto unregister_notifier;
> + }
> +
> err = ds->ops->setup(ds);
> if (err < 0)
> goto unregister_notifier;
>
> devlink_params_publish(ds->devlink);
>
> - if (!ds->slave_mii_bus && ds->ops->phy_read) {
> - ds->slave_mii_bus = devm_mdiobus_alloc(ds->dev);
> - if (!ds->slave_mii_bus) {
> - err = -ENOMEM;
> - goto unregister_notifier;
> - }
> -
> + if (ds->ops->phy_read) {
> dsa_slave_mii_bus_init(ds);
>
> err = mdiobus_register(ds->slave_mii_bus);
>
Powered by blists - more mailing lists