Skip to contentSkip to conversation

usb: chipidea: tegra: Fix missed ci_hdrc_remove_device()

PATCH In review Dmitry Osipenko 3 participants 9 messages Feb 24, 2019 – Feb 26, 2019

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 usb: chipidea: tegra: Fix missed ci_hdrc_remove_device()

The ChipIdea's platform device need to be unregistered on Tegra's driver module removal.

Fixes dfebb5f43a78827a ("usb: chipidea: Add support for Tegra20/30/114/124")
---
 drivers/usb/chipidea/ci_hdrc_tegra.c | 1 +
 1 file changed, 1 insertion(+)
drivers/usb/chipidea/ci_hdrc_tegra.c +1 -0
index 772851bee99b..12025358bb3c 100644
@@ -130,6 +130,7 @@ static int tegra_udc_remove(struct platform_device *pdev)
 {
 	struct tegra_udc *udc = platform_get_drvdata(pdev);
 
+	ci_hdrc_remove_device(udc->dev);
 	usb_phy_set_suspend(udc->phy, 1);
 	clk_disable_unprepare(udc->clk);
 
Signature · 1 line
2.20.1
Acked-by on v1 1/1
#1 lore raw
Quoted 3 lines
Fixes dfebb5f43a78827a ("usb: chipidea: Add support for Tegra20/30/114/124")

I suppose you need to apply at stable tree too, right?

Quoted 11 lines
---
 drivers/usb/chipidea/ci_hdrc_tegra.c | 1 +
 1 file changed, 1 insertion(+)
drivers/usb/chipidea/ci_hdrc_tegra.c +0 -0
index 772851bee99b..12025358bb3c 100644

drivers/usb/chipidea/ci_hdrc_tegra.c static int tegra_udc_remove(struct platform_device *pdev) · line 136

Quoted 7 lines
drivers/usb/chipidea/ci_hdrc_tegra.c +1 -0
@@ -130,6 +130,7 @@ static int tegra_udc_remove(struct platform_device *pdev)
 {
 	struct tegra_udc *udc = platform_get_drvdata(pdev);
 
+	ci_hdrc_remove_device(udc->dev);
 	usb_phy_set_suspend(udc->phy, 1);
 	clk_disable_unprepare(udc->clk);
 

Hi Greg, would you still accept the bug-fix for this release (v5.0)? Or I send you later?

Peter

#2 lore raw

В Mon, 25 Feb 2019 02:27:19 +0000
Peter Chen <[email protected]> пишет:

Quoted 7 lines
Quoted 4 lines
Fixes dfebb5f43a78827a ("usb: chipidea: Add support for

Tegra20/30/114/124")

I suppose you need to apply at stable tree too, right?

It is enough to have the "Fixes" tag to get patch backported into all relevant kernel versions.

Quoted 21 lines
Quoted 11 lines
---
 drivers/usb/chipidea/ci_hdrc_tegra.c | 1 +
 1 file changed, 1 insertion(+)
drivers/usb/chipidea/ci_hdrc_tegra.c +0 -0
index 772851bee99b..12025358bb3c 100644

drivers/usb/chipidea/ci_hdrc_tegra.c static int tegra_udc_remove(struct · line 136

Quoted 7 lines
drivers/usb/chipidea/ci_hdrc_tegra.c +1 -0
@@ -130,6 +130,7 @@ static int tegra_udc_remove(struct
 platform_device *pdev) {
 	struct tegra_udc *udc = platform_get_drvdata(pdev);
 
+	ci_hdrc_remove_device(udc->dev);
 	usb_phy_set_suspend(udc->phy, 1);
 	clk_disable_unprepare(udc->clk);
 
Quoted 2 lines

Hi Greg, would you still accept the bug-fix for this release (v5.0)? Or I send you later?

I just want to point out that the bug existed for a very long time and apparently didn't bother anybody, hence it's not an urgent fix.

#3 lore raw
Quoted 5 lines · Dmitry Osipenko

В Mon, 25 Feb 2019 02:27:19 +0000
Peter Chen <[email protected]> пишет:

Quoted 2 lines
Quoted 2 lines
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
Fixes dfebb5f43a78827a ("usb: chipidea: Add support for

Tegra20/30/114/124")

I suppose you need to apply at stable tree too, right?

It is enough to have the "Fixes" tag to get patch backported into all relevant kernel versions.

No it is not. My scripts do NOT trigger off of the fixes: tag, please read:
https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

Quoted 21 lines
Quoted 21 lines
Quoted 11 lines
---
 drivers/usb/chipidea/ci_hdrc_tegra.c | 1 +
 1 file changed, 1 insertion(+)
drivers/usb/chipidea/ci_hdrc_tegra.c +0 -0
index 772851bee99b..12025358bb3c 100644

drivers/usb/chipidea/ci_hdrc_tegra.c static int tegra_udc_remove(struct · line 136

Quoted 7 lines
drivers/usb/chipidea/ci_hdrc_tegra.c +1 -0
@@ -130,6 +130,7 @@ static int tegra_udc_remove(struct
 platform_device *pdev) {
 	struct tegra_udc *udc = platform_get_drvdata(pdev);
 
+	ci_hdrc_remove_device(udc->dev);
 	usb_phy_set_suspend(udc->phy, 1);
 	clk_disable_unprepare(udc->clk);
 
Quoted 2 lines
Quoted 2 lines

Hi Greg, would you still accept the bug-fix for this release (v5.0)? Or I send you later?

I can pick it up now, thanks.

greg k-h

#4 lore raw

26.02.2019 13:56, Greg Kroah-Hartman пишет:

Quoted 12 lines · Dmitry Osipenko
Quoted 11 lines

В Mon, 25 Feb 2019 02:27:19 +0000
Peter Chen <[email protected]> пишет:

Quoted 7 lines
Quoted 4 lines
Fixes dfebb5f43a78827a ("usb: chipidea: Add support for

Tegra20/30/114/124")

I suppose you need to apply at stable tree too, right?

Quoted 7 lines
Quoted 2 lines

It is enough to have the "Fixes" tag to get patch backported into all relevant kernel versions.

No it is not. My scripts do NOT trigger off of the fixes: tag, please read:
https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

Okay, my bad then. Maybe this is something that could warned by checkpatch.. adding Joe and Andy to the thread.

#5 lore raw
Quoted 16 lines · Dmitry Osipenko

26.02.2019 13:56, Greg Kroah-Hartman пишет:

Quoted 15 lines · Dmitry Osipenko
Quoted 13 lines

В Mon, 25 Feb 2019 02:27:19 +0000
Peter Chen <[email protected]> пишет:

Quoted 7 lines
Quoted 4 lines
Fixes dfebb5f43a78827a ("usb: chipidea: Add support for

Tegra20/30/114/124")

I suppose you need to apply at stable tree too, right?

It is enough to have the "Fixes" tag to get patch backported into all relevant kernel versions.

Quoted 6 lines
Quoted 4 lines

No it is not. My scripts do NOT trigger off of the fixes: tag, please read:
https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

Okay, my bad then. Maybe this is something that could warned by checkpatch.. adding Joe and Andy to the thread.

Why? It's allowed to put fixes: tags for a patch that does not belong in a stable tree. That happens all the time, and is encouraged. Look at some of the stuff in linux-next now, we have Fixes: for commits that are still in linux-next as well, because we do not rebase our trees. When they all merge into Linus's tree, all is good.

So this is not something that checkpatch needs to do anything about.

thanks,

greg k-h

#6 lore raw

26.02.2019 17:58, Greg Kroah-Hartman пишет:

Quoted 24 lines · Dmitry Osipenko
Quoted 16 lines

26.02.2019 13:56, Greg Kroah-Hartman пишет:

Quoted 15 lines · Dmitry Osipenko
Quoted 13 lines

В Mon, 25 Feb 2019 02:27:19 +0000
Peter Chen <[email protected]> пишет:

Quoted 7 lines
Quoted 4 lines
Fixes dfebb5f43a78827a ("usb: chipidea: Add support for

Tegra20/30/114/124")

I suppose you need to apply at stable tree too, right?

It is enough to have the "Fixes" tag to get patch backported into all relevant kernel versions.

Quoted 6 lines
Quoted 4 lines

No it is not. My scripts do NOT trigger off of the fixes: tag, please read:
https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

Okay, my bad then. Maybe this is something that could warned by checkpatch.. adding Joe and Andy to the thread.

Quoted 7 lines

Why? It's allowed to put fixes: tags for a patch that does not belong in a stable tree. That happens all the time, and is encouraged. Look at some of the stuff in linux-next now, we have Fixes: for commits that are still in linux-next as well, because we do not rebase our trees. When they all merge into Linus's tree, all is good.

So this is not something that checkpatch needs to do anything about.

At least that might help in cases like this if maintainer is also oblivious.

I guess it wouldn't hurt at all to add a "green" warning if there is a "Fixes" tag and no "stable", something like this:

WARNING: There is a "Fixes" tag and no "Cc: stable <[email protected]>", please add the stable tag if patch is intended to be backported to stable kernels.

#7 lore raw
Quoted 31 lines · Dmitry Osipenko

26.02.2019 17:58, Greg Kroah-Hartman пишет:

Quoted 30 lines · Dmitry Osipenko
Quoted 16 lines

26.02.2019 13:56, Greg Kroah-Hartman пишет:

Quoted 15 lines · Dmitry Osipenko
Quoted 13 lines

В Mon, 25 Feb 2019 02:27:19 +0000
Peter Chen <[email protected]> пишет:

Quoted 7 lines
Quoted 4 lines
Fixes dfebb5f43a78827a ("usb: chipidea: Add support for

Tegra20/30/114/124")

I suppose you need to apply at stable tree too, right?

It is enough to have the "Fixes" tag to get patch backported into all relevant kernel versions.

Quoted 6 lines
Quoted 4 lines

No it is not. My scripts do NOT trigger off of the fixes: tag, please read:
https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

Okay, my bad then. Maybe this is something that could warned by checkpatch.. adding Joe and Andy to the thread.

Why? It's allowed to put fixes: tags for a patch that does not belong in a stable tree. That happens all the time, and is encouraged. Look at some of the stuff in linux-next now, we have Fixes: for commits that are still in linux-next as well, because we do not rebase our trees. When they all merge into Linus's tree, all is good.

Quoted 3 lines
Quoted 1 line

So this is not something that checkpatch needs to do anything about.

At least that might help in cases like this if maintainer is also oblivious.

If the maintainer is "oblivious", they are not going to be running checkpatch :)

Remember, the "Fixes:" tag is a relatively new thing compared to the cc:
stable tag, which has been a documented requirement for over a decade. Yes, some subsystems do not even do cc: stable, but that is because those subsystem maintainers do not want to do it, or do not care. Again, checkpatch is not going to help them.

checkpatch is not a panacea, people still have to use their brains.

greg k-h

#8 lore raw

26.02.2019 18:21, Greg Kroah-Hartman пишет:

Quoted 39 lines · Dmitry Osipenko
Quoted 31 lines

26.02.2019 17:58, Greg Kroah-Hartman пишет:

Quoted 30 lines · Dmitry Osipenko
Quoted 16 lines

26.02.2019 13:56, Greg Kroah-Hartman пишет:

Quoted 15 lines · Dmitry Osipenko
Quoted 13 lines

В Mon, 25 Feb 2019 02:27:19 +0000
Peter Chen <[email protected]> пишет:

Quoted 7 lines
Quoted 4 lines
Fixes dfebb5f43a78827a ("usb: chipidea: Add support for

Tegra20/30/114/124")

I suppose you need to apply at stable tree too, right?

It is enough to have the "Fixes" tag to get patch backported into all relevant kernel versions.

Quoted 6 lines
Quoted 4 lines

No it is not. My scripts do NOT trigger off of the fixes: tag, please read:
https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

Okay, my bad then. Maybe this is something that could warned by checkpatch.. adding Joe and Andy to the thread.

Why? It's allowed to put fixes: tags for a patch that does not belong in a stable tree. That happens all the time, and is encouraged. Look at some of the stuff in linux-next now, we have Fixes: for commits that are still in linux-next as well, because we do not rebase our trees. When they all merge into Linus's tree, all is good.

Quoted 3 lines
Quoted 1 line

So this is not something that checkpatch needs to do anything about.

At least that might help in cases like this if maintainer is also oblivious.

If the maintainer is "oblivious", they are not going to be running checkpatch :)

Quoted 7 lines

Remember, the "Fixes:" tag is a relatively new thing compared to the cc:
stable tag, which has been a documented requirement for over a decade. Yes, some subsystems do not even do cc: stable, but that is because those subsystem maintainers do not want to do it, or do not care. Again, checkpatch is not going to help them.

checkpatch is not a panacea, people still have to use their brains.

My point that either submitter or maintainers may catch up the problem if at least there are means to report the problem. So I'm still thinking it's not a bad idea to add the warning message, maybe even for the "checkpatch --strict" option only. I'm not insisting that it is something super-necessary and it's up to you guys to decide in the end.