netdev - Re: [Patch net-next v2 4/5] net: dsa: microchip: move interrupt handling logic from lan937x to ksz

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] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 14 Sep 2022 12:27:39 +0100
From:   "Russell King (Oracle)" <linux@...linux.org.uk>
To:     Arun Ramadoss <arun.ramadoss@...rochip.com>
Cc:     linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
        woojung.huh@...rochip.com, UNGLinuxDriver@...rochip.com,
        andrew@...n.ch, vivien.didelot@...il.com, f.fainelli@...il.com,
        olteanv@...il.com, davem@...emloft.net, edumazet@...gle.com,
        kuba@...nel.org, pabeni@...hat.com, Tristram.Ha@...rochip.com,
        prasanna.vengateshan@...rochip.com, hkallweit1@...il.com
Subject: Re: [Patch net-next v2 4/5] net: dsa: microchip: move interrupt
 handling logic from lan937x to ksz_common

Hi,

Some suggestions for a few improvements in a future patch:

On Wed, Sep 14, 2022 at 09:22:22AM +0530, Arun Ramadoss wrote:
> +static int ksz_irq_phy_setup(struct ksz_device *dev)
> +{
> +	struct dsa_switch *ds = dev->ds;
> +	int phy, err_phy;
> +	int irq;
> +	int ret;
> +
> +	for (phy = 0; phy < KSZ_MAX_NUM_PORTS; phy++) {
> +		if (BIT(phy) & ds->phys_mii_mask) {
> +			irq = irq_find_mapping(dev->ports[phy].pirq.domain,
> +					       PORT_SRC_PHY_INT);
> +			if (irq < 0) {
> +				ret = irq;
> +				goto out;
> +			}
> +			ds->slave_mii_bus->irq[phy] = irq;
> +		}
> +	}
> +	return 0;
> +out:
> +	err_phy = phy;
> +
> +	for (phy = 0; phy < err_phy; phy++)
> +		if (BIT(phy) & ds->phys_mii_mask)
> +			irq_dispose_mapping(ds->slave_mii_bus->irq[phy]);

	while (phy--)
		if (BIT(phy) & ds->phys_mii_mask)
			irq_dispose_mapping(ds->slave_mii_bus->irq[phy]);

?

> +static void ksz_girq_mask(struct irq_data *d)
> +{
> +	struct ksz_device *dev = irq_data_get_irq_chip_data(d);
> +	unsigned int n = d->hwirq;
> +
> +	dev->girq.masked |= (1 << n);

	dev->girq.masked |= BIT(d->hwirq);

?

> +}
> +
> +static void ksz_girq_unmask(struct irq_data *d)
> +{
> +	struct ksz_device *dev = irq_data_get_irq_chip_data(d);
> +	unsigned int n = d->hwirq;
> +
> +	dev->girq.masked &= ~(1 << n);

	dev->girq.masked &= ~BIT(d->hw_irq);

?

> +}
> +
> +static void ksz_girq_bus_lock(struct irq_data *d)
> +{
> +	struct ksz_device *dev = irq_data_get_irq_chip_data(d);
> +
> +	mutex_lock(&dev->lock_irq);
> +}
> +
> +static void ksz_girq_bus_sync_unlock(struct irq_data *d)
> +{
> +	struct ksz_device *dev = irq_data_get_irq_chip_data(d);
> +	int ret;
> +
> +	ret = ksz_write32(dev, REG_SW_PORT_INT_MASK__4, dev->girq.masked);
> +	if (ret)
> +		dev_err(dev->dev, "failed to change IRQ mask\n");
> +
> +	mutex_unlock(&dev->lock_irq);
> +}
> +
> +static const struct irq_chip ksz_girq_chip = {
> +	.name			= "ksz-global",
> +	.irq_mask		= ksz_girq_mask,
> +	.irq_unmask		= ksz_girq_unmask,
> +	.irq_bus_lock		= ksz_girq_bus_lock,
> +	.irq_bus_sync_unlock	= ksz_girq_bus_sync_unlock,
> +};

As the pirq code is almost identical to the girq code, how about putting
a "reg_mask", "reg_status" and a pointer to ksz_device into ksz_irq, and
using the ksz_irq as the chip data?

These would then become:

static void ksz_irq_mask(struct irq_data *d)
{
	struct ksz_irq *ki = irq_data_get_irq_chip_data(d);

	ki->masked |= BIT(d->hwirq);
}

static void ksz_irq_unmask(struct irq_data *d)
{
	struct ksz_irq *ki = irq_data_get_irq_chip_data(d);

	ki->masked &= ~BIT(d->hwirq);
}

static void ksz_irq_bus_lock(struct irq_data *d)
{
	struct ksz_irq *ki = irq_data_get_irq_chip_data(d);

	mutex_lock(&ki->dev->lock_irq);
}

static void ksz_irq_bus_sync_unlock(struct irq_data *d)
{
	struct ksz_irq *ki = irq_data_get_irq_chip_data(d);
	struct ksz_device *dev = ki->dev;
	int ret;

	ret = ksz_write32(dev, ki->reg_masked, ki->masked);
	if (ret)
		dev_err(dev->dev, "failed to change IRQ mask\n");

	mutex_unlock(&dev->lock_irq);
}

and thus this code could be shared between both pirq and girq.
I'm pretty sure the thead_fn could be shared as well, and I'm
sure that the setup and tear down could be improved in a similar
way.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!