linux-kernel - Re: [PATCH net-next 1/2] net: renesas: rswitch: use per-port irq handlers

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

Message-ID: <Z2jy1EglD4j/ENvM@mev-dev.igk.intel.com>
Date: Mon, 23 Dec 2024 06:19:16 +0100
From: Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>
To: Nikita Yushchenko <nikita.yoush@...entembedded.com>
Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>,
	Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Geert Uytterhoeven <geert+renesas@...der.be>,
	netdev@...r.kernel.org, linux-renesas-soc@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	Michael Dege <michael.dege@...esas.com>,
	Christian Mardmoeller <christian.mardmoeller@...esas.com>,
	Dennis Ostermann <dennis.ostermann@...esas.com>
Subject: Re: [PATCH net-next 1/2] net: renesas: rswitch: use per-port irq
 handlers

On Fri, Dec 20, 2024 at 02:11:26PM +0500, Nikita Yushchenko wrote:
> > > +	ret = request_irq(rdev->irq, rswitch_gwca_data_irq, IRQF_SHARED,
> > It wasn't shared previously, maybe some notes in commit message about
> > that.
> 
> It can be shared between several ports.
> 
> I will try to rephrase the commit message to make this stated explicitly.
> 
> > > +	err = of_property_read_u32(rdev->np_port, "irq-index", &irq_index);
> > > +	if (err == 0) {
> > Usually if (!err) is used.
> 
> Ok, will fix it.
> 
> > 
> > > +		if (irq_index < GWCA_NUM_IRQS)
> > > +			rdev->irq_index = irq_index;
> > > +		else
> > > +			dev_warn(&rdev->priv->pdev->dev,
> > > +				 "%pOF: irq-index out of range\n",
> > > +				 rdev->np_port);
> > Why not return here? It is a little counter intuitive, maybe:
> > if (err) {
> > 	dev_warn();
> > 	return -ERR;
> > }
> 
> It is meant to be optional, not having it defined shall not be an error
> 
> > if (irq_index < NUM_IRQS) {
> > 	dev_warn();
> > 	return -ERR;
> > }
> 
> Ok - although if erroring out, I think it shall be dev_err.
> 
> > > +	}
> > > +
> > > +	name = kasprintf(GFP_KERNEL, GWCA_IRQ_RESOURCE_NAME, rdev->irq_index);
> > 
> > In case with not returning you are using invalid rdev_irq_index here
> > (probably 0, so may it be fine, I am only wondering).
> 
> Yes, the field is zero-initialized and that zero is a sane default.
> 
> > 
> > > +	if (!name)
> > > +		return -ENOMEM;
> > > +	err = platform_get_irq_byname(rdev->priv->pdev, name);
> > > +	kfree(name);
> > > +	if (err < 0)
> > > +		return err;
> > > +	rdev->irq = err;
> > 
> > If you will be changing sth here consider:
> > rdev->irq = platform()
> > if (rdev->irq < 0)
> > 	return rdev->irq;
> 
> Ok
> 
> > > +	err = rswitch_port_get_irq(rdev);
> > > +	if (err < 0)
> > You are returning 0 in case of success, the netdev code style is to
> > check it like that: if (!err)
> 
> I tried to follow the style already existing in the driver.
> Several checks just above and below are written this way.
> Shall I add this one check written differently?
> 

I am fine with following exsisting style.

Thanks

> > 
> > > +		goto out_get_irq;
> > If you will use the label name according to what does happen under label
> > you will not have to add another one. Feel free to leave it as it is, as
> > you have the same scheme across driver with is completle fine. You can
> > check Przemek's answer according "came from" convention [1].
> 
> Again, following existing style here.
> 
> My personal opinion is that "came from" labels are more reliable against
> future changes than other label styles. But if there is maintainer
> requirement here then definitely I will follow.
> 
> Nikita