Hello Claudiu-san,
> From: Claudiu, Sent: Thursday, February 20, 2025 1:08 AM
>
> From: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
>
> Commit 08b0ad375ca6 ("phy: renesas: rcar-gen3-usb2: move IRQ registration
> to init") moved the IRQ request operation from probe to
I don't know why the checkpatch.pl said the following ERROR though,
as I sent my Reviewed-by on the top of this email thread, this patch is good,
I think.
---
ERROR: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'Commit 08b0ad375ca6 ("phy: renesas: rcar-gen3-usb2: move irq registration to init")'
#6:
Commit 08b0ad375ca6 ("phy: renesas: rcar-gen3-usb2: move IRQ registration
to init") moved the IRQ request operation from probe to
---
Best regards,
Yoshihiro Shimoda
> struct phy_ops::phy_init API to avoid triggering interrupts (which lead to
> register accesses) while the PHY clocks (enabled through runtime PM APIs)
> are not active. If this happens, it results in a synchronous abort.
>
> One way to reproduce this issue is by enabling CONFIG_DEBUG_SHIRQ, which
> calls free_irq() on driver removal.
>
> Move the IRQ request and free operations back to probe, and take the
> runtime PM state into account in IRQ handler. This commit is preparatory
> for the subsequent fixes in this series.
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
> ---
> drivers/phy/renesas/phy-rcar-gen3-usb2.c | 46 +++++++++++++-----------
> 1 file changed, 26 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> index 46afba2fe0dc..826c9c4dd4c0 100644
> --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> @@ -120,7 +120,6 @@ struct rcar_gen3_chan {
> struct work_struct work;
> struct mutex lock; /* protects rphys[...].powered */
> enum usb_dr_mode dr_mode;
> - int irq;
> u32 obint_enable_bits;
> bool extcon_host;
> bool is_otg_channel;
> @@ -428,16 +427,25 @@ static irqreturn_t rcar_gen3_phy_usb2_irq(int irq, void *_ch)
> {
> struct rcar_gen3_chan *ch = _ch;
> void __iomem *usb2_base = ch->base;
> - u32 status = readl(usb2_base + USB2_OBINTSTA);
> + struct device *dev = ch->dev;
> irqreturn_t ret = IRQ_NONE;
> + u32 status;
>
> + pm_runtime_get_noresume(dev);
> +
> + if (pm_runtime_suspended(dev))
> + goto rpm_put;
> +
> + status = readl(usb2_base + USB2_OBINTSTA);
> if (status & ch->obint_enable_bits) {
> - dev_vdbg(ch->dev, "%s: %08x\n", __func__, status);
> + dev_vdbg(dev, "%s: %08x\n", __func__, status);
> writel(ch->obint_enable_bits, usb2_base + USB2_OBINTSTA);
> rcar_gen3_device_recognition(ch);
> ret = IRQ_HANDLED;
> }
>
> +rpm_put:
> + pm_runtime_put_noidle(dev);
> return ret;
> }
>
> @@ -447,17 +455,6 @@ static int rcar_gen3_phy_usb2_init(struct phy *p)
> struct rcar_gen3_chan *channel = rphy->ch;
> void __iomem *usb2_base = channel->base;
> u32 val;
> - int ret;
> -
> - if (!rcar_gen3_is_any_rphy_initialized(channel) && channel->irq >= 0) {
> - INIT_WORK(&channel->work, rcar_gen3_phy_usb2_work);
> - ret = request_irq(channel->irq, rcar_gen3_phy_usb2_irq,
> - IRQF_SHARED, dev_name(channel->dev), channel);
> - if (ret < 0) {
> - dev_err(channel->dev, "No irq handler (%d)\n", channel->irq);
> - return ret;
> - }
> - }
>
> /* Initialize USB2 part */
> val = readl(usb2_base + USB2_INT_ENABLE);
> @@ -490,9 +487,6 @@ static int rcar_gen3_phy_usb2_exit(struct phy *p)
> val &= ~USB2_INT_ENABLE_UCOM_INTEN;
> writel(val, usb2_base + USB2_INT_ENABLE);
>
> - if (channel->irq >= 0 && !rcar_gen3_is_any_rphy_initialized(channel))
> - free_irq(channel->irq, channel);
> -
> return 0;
> }
>
> @@ -698,7 +692,7 @@ static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev)
> struct device *dev = &pdev->dev;
> struct rcar_gen3_chan *channel;
> struct phy_provider *provider;
> - int ret = 0, i;
> + int ret = 0, i, irq;
>
> if (!dev->of_node) {
> dev_err(dev, "This driver needs device tree\n");
> @@ -714,8 +708,6 @@ static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev)
> return PTR_ERR(channel->base);
>
> channel->obint_enable_bits = USB2_OBINT_BITS;
> - /* get irq number here and request_irq for OTG in phy_init */
> - channel->irq = platform_get_irq_optional(pdev, 0);
> channel->dr_mode = rcar_gen3_get_dr_mode(dev->of_node);
> if (channel->dr_mode != USB_DR_MODE_UNKNOWN) {
> channel->is_otg_channel = true;
> @@ -784,6 +776,20 @@ static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev)
> channel->vbus = NULL;
> }
>
> + irq = platform_get_irq_optional(pdev, 0);
> + if (irq == -EPROBE_DEFER) {
> + ret = -EPROBE_DEFER;
> + goto error;
> + } else if (irq >= 0) {
> + INIT_WORK(&channel->work, rcar_gen3_phy_usb2_work);
> + ret = devm_request_irq(dev, irq, rcar_gen3_phy_usb2_irq,
> + IRQF_SHARED, dev_name(dev), channel);
> + if (ret < 0) {
> + dev_err(dev, "Failed to request irq (%d)\n", irq);
> + goto error;
> + }
> + }
> +
> provider = devm_of_phy_provider_register(dev, rcar_gen3_phy_usb2_xlate);
> if (IS_ERR(provider)) {
> dev_err(dev, "Failed to register PHY provider\n");
> --
> 2.43.0
