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

Message-ID: <0e95c4dc-e155-4860-b918-13e47bf9b9c6@cogentembedded.com>
Date: Fri, 20 Dec 2024 14:11:26 +0500
From: Nikita Yushchenko <nikita.yoush@...entembedded.com>
To: Michal Swiatkowski <michal.swiatkowski@...ux.intel.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

>> +	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?

> 
>> +		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