| 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: <5536e607-e03b-f38e-2909-a6f6a126ff0d@omp.ru>
Date: Wed, 31 Jan 2024 22:51:28 +0300
From: Sergey Shtylyov <s.shtylyov@....ru>
To: Claudiu <claudiu.beznea@...on.dev>, <davem@...emloft.net>,
<edumazet@...gle.com>, <kuba@...nel.org>, <pabeni@...hat.com>,
<richardcochran@...il.com>, <p.zabel@...gutronix.de>,
<geert+renesas@...der.be>
CC: <netdev@...r.kernel.org>, <linux-renesas-soc@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, Claudiu Beznea
<claudiu.beznea.uj@...renesas.com>
Subject: Re: [PATCH net-next v5 08/15] net: ravb: Move the IRQs
getting/requesting in the probe() method
I said the subject needs to be changed to "net: ravb: Move getting/requesting IRQs in
the probe() method", not "net: ravb: Move IRQs getting/requesting in the probe() method".
That's not very proper English, AFAIK! =)
On 1/31/24 11:41 AM, Claudiu wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
>
> The runtime PM implementation will disable clocks at the end of
> ravb_probe(). As some IP variants switch to reset mode as a result of
> setting module standby through clock disable APIs, to implement runtime PM
> the resource parsing and requesting are moved in the probe function and IP
> settings are moved in the open function. This is done because at the end of
> the probe some IP variants will switch anyway to reset mode and the
> registers content is lost. Also keeping only register settings operations
> in the ravb_open()/ravb_close() functions will make them faster.
>
> Commit moves IRQ requests to ravb_probe() to have all the IRQs ready when
> the interface is open. As now IRQs getting/requesting are in a single place
Again, "getting/requesting IRQs is done"...
> there is no need to keep intermediary data (like ravb_rx_irqs[] and
> ravb_tx_irqs[] arrays or IRQs in struct ravb_private).
>
> In order to avoid accessing the IP registers while the IP is runtime
> suspended (e.g. in the timeframe b/w the probe requests shared IRQs and
> IP clocks are enabled) in the interrupt handlers were introduced
But can't we just request our IRQs after we call pm_runtime_resume_and_get()?
We proaobly can... but then again, we call pm_runtime_put_sync() in the remove()
method before the IRQs are freed... So, it still seems OK that we're adding
pm_runtime_active() calls to the IRQ handlers in this very patch, not when we'll
start calling the RPM APIs in the ndo_{open|close}() methods, correct? :-)
> pm_runtime_active() checks. The device runtime PM usage counter has been
> incremented to avoid disabling the device's clocks while the check is in
> progress (if any).
>
> This is a preparatory change to add runtime PM support for all IP variants.
>
> Reviewed-by: Sergey Shtylyov <s.shtylyov@....ru>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
[...]
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index e70c930840ce..f9297224e527 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
[...]
> @@ -1092,11 +1082,23 @@ static irqreturn_t ravb_emac_interrupt(int irq, void *dev_id)
> {
> struct net_device *ndev = dev_id;
> struct ravb_private *priv = netdev_priv(ndev);
> + struct device *dev = &priv->pdev->dev;
> + irqreturn_t result = IRQ_HANDLED;
> +
> + pm_runtime_get_noresume(dev);
> +
Not sure we need en empty line here...
> + if (unlikely(!pm_runtime_active(dev))) {
> + result = IRQ_NONE;
> + goto out_rpm_put;
> + }
>
> spin_lock(&priv->lock);
> ravb_emac_interrupt_unlocked(ndev);
> spin_unlock(&priv->lock);
> - return IRQ_HANDLED;
> +
> +out_rpm_put:
> + pm_runtime_put_noidle(dev);
> + return result;
> }
>
> /* Error interrupt handler */
> @@ -1176,9 +1178,15 @@ static irqreturn_t ravb_interrupt(int irq, void *dev_id)
> struct net_device *ndev = dev_id;
> struct ravb_private *priv = netdev_priv(ndev);
> const struct ravb_hw_info *info = priv->info;
> + struct device *dev = &priv->pdev->dev;
> irqreturn_t result = IRQ_NONE;
> u32 iss;
>
> + pm_runtime_get_noresume(dev);
> +
And here...
> + if (unlikely(!pm_runtime_active(dev)))
> + goto out_rpm_put;
> +
> spin_lock(&priv->lock);
> /* Get interrupt status */
> iss = ravb_read(ndev, ISS);
[...]
> @@ -1230,9 +1241,15 @@ static irqreturn_t ravb_multi_interrupt(int irq, void *dev_id)
> {
> struct net_device *ndev = dev_id;
> struct ravb_private *priv = netdev_priv(ndev);
> + struct device *dev = &priv->pdev->dev;
> irqreturn_t result = IRQ_NONE;
> u32 iss;
>
> + pm_runtime_get_noresume(dev);
> +
Here too...
> + if (unlikely(!pm_runtime_active(dev)))
> + goto out_rpm_put;
> +
> spin_lock(&priv->lock);
> /* Get interrupt status */
> iss = ravb_read(ndev, ISS);
[...]
> @@ -1261,8 +1281,14 @@ static irqreturn_t ravb_dma_interrupt(int irq, void *dev_id, int q)
> {
> struct net_device *ndev = dev_id;
> struct ravb_private *priv = netdev_priv(ndev);
> + struct device *dev = &priv->pdev->dev;
> irqreturn_t result = IRQ_NONE;
>
> + pm_runtime_get_noresume(dev);
> +
Here as well...
> + if (unlikely(!pm_runtime_active(dev)))
> + goto out_rpm_put;
> +
> spin_lock(&priv->lock);
>
> /* Network control/Best effort queue RX/TX */
[...]
> @@ -2616,6 +2548,90 @@ static void ravb_parse_delay_mode(struct device_node *np, struct net_device *nde
> }
> }
>
> +static int ravb_setup_irq(struct ravb_private *priv, const char *irq_name,
> + const char *ch, int *irq, irq_handler_t handler)
> +{
> + struct platform_device *pdev = priv->pdev;
> + struct net_device *ndev = priv->ndev;
> + struct device *dev = &pdev->dev;
> + const char *dev_name;
> + unsigned long flags;
> + int error;
> +
> + if (irq_name) {
> + dev_name = devm_kasprintf(dev, GFP_KERNEL, "%s:%s", ndev->name, ch);
> + if (!dev_name)
> + return -ENOMEM;
> +
> + *irq = platform_get_irq_byname(pdev, irq_name);
> + flags = 0;
> + } else {
> + dev_name = ndev->name;
> + *irq = platform_get_irq(pdev, 0);
> + flags = IRQF_SHARED;
Perhaps it's worth passing flags as a parameter here instead?
> + }
> + if (*irq < 0)
> + return *irq;
> +
> + error = devm_request_irq(dev, *irq, handler, flags, dev_name, ndev);
> + if (error)
> + netdev_err(ndev, "cannot request IRQ %s\n", dev_name);
> +
> + return error;
> +}
[...]
MBR, Sergey
Powered by blists - more mailing lists