From: Adrian Hunter <adrian.hunter@intel.com>
To: Bart Van Assche <bvanassche@acm.org>,
"Martin K . Petersen" <martin.petersen@oracle.com>
Cc: Jaegeuk Kim <jaegeuk@kernel.org>,
Avri Altman <avri.altman@wdc.com>,
linux-scsi@vger.kernel.org,
"James E.J. Bottomley" <jejb@linux.ibm.com>,
Alim Akhtar <alim.akhtar@samsung.com>,
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
Stanley Chu <stanley.chu@mediatek.com>,
Andy Gross <agross@kernel.org>,
Bjorn Andersson <andersson@kernel.org>,
Manivannan Sadhasivam <mani@kernel.org>,
Orson Zhai <orsonzhai@gmail.com>,
Baolin Wang <baolin.wang@linux.alibaba.com>,
Chunyan Zhang <zhang.lyra@gmail.com>,
Matthias Brugger <matthias.bgg@gmail.com>,
Bean Huo <beanhuo@micron.com>,
Asutosh Das <quic_asutoshd@quicinc.com>,
Zhe Wang <zhe.wang1@unisoc.com>,
Daniil Lunev <dlunev@chromium.org>, Liang He <windhl@126.com>,
Can Guo <quic_cang@quicinc.com>
Subject: Re: [PATCH 4/4] scsi: ufs: Simplify driver shutdown
Date: Wed, 26 Apr 2023 09:07:15 +0300 [thread overview]
Message-ID: <91efe449-0000-8e1a-afb8-2628d46dd245@intel.com> (raw)
In-Reply-To: <20230425232954.1229916-5-bvanassche@acm.org>
On 26/04/23 02:29, Bart Van Assche wrote:
> All UFS host drivers call ufshcd_shutdown(). Hence, instead of calling
> ufshcd_shutdown() from the host driver .shutdown() callback, inline that
> function into ufshcd_wl_shutdown().
Pedantically, this seems like a bit of a layering violation: the child
is doing the parent's shutdown. It assumes host shutdown is not needed
if ufs_device_wlun does not exist or did not successfully probe.
At least it could use a comment in the code.
>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
> drivers/ufs/core/ufshcd.c | 15 ---------------
> drivers/ufs/host/cdns-pltfrm.c | 1 -
> drivers/ufs/host/tc-dwc-g210-pci.c | 10 ----------
> drivers/ufs/host/tc-dwc-g210-pltfrm.c | 1 -
> drivers/ufs/host/ufs-exynos.c | 1 -
> drivers/ufs/host/ufs-hisi.c | 1 -
> drivers/ufs/host/ufs-mediatek.c | 1 -
> drivers/ufs/host/ufs-qcom.c | 1 -
> drivers/ufs/host/ufs-sprd.c | 1 -
> drivers/ufs/host/ufshcd-pci.c | 10 ----------
> drivers/ufs/host/ufshcd-pltfrm.c | 6 ------
> drivers/ufs/host/ufshcd-pltfrm.h | 1 -
> include/ufs/ufshcd.h | 1 -
> 13 files changed, 50 deletions(-)
>
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 215a7f713b46..7b1e7d7091ff 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -9965,27 +9965,12 @@ static void ufshcd_wl_shutdown(struct device *dev)
> scsi_device_quiesce(sdev);
> }
> __ufshcd_wl_suspend(hba, UFS_SHUTDOWN_PM);
> -}
>
> -/**
> - * ufshcd_shutdown - shutdown routine
> - * @hba: per adapter instance
> - *
> - * This function would turn off both UFS device and UFS hba
> - * regulators. It would also disable clocks.
> - *
> - * Returns 0 always to allow force shutdown even in case of errors.
> - */
> -int ufshcd_shutdown(struct ufs_hba *hba)
> -{
> if (ufshcd_is_ufs_dev_poweroff(hba) && ufshcd_is_link_off(hba))
> ufshcd_suspend(hba);
>
> hba->is_powered = false;
> - /* allow force shutdown even in case of errors */
> - return 0;
> }
> -EXPORT_SYMBOL(ufshcd_shutdown);
>
> /**
> * ufshcd_remove - de-allocate SCSI host and host memory space
> diff --git a/drivers/ufs/host/cdns-pltfrm.c b/drivers/ufs/host/cdns-pltfrm.c
> index e05c0ae64eea..26761425a76c 100644
> --- a/drivers/ufs/host/cdns-pltfrm.c
> +++ b/drivers/ufs/host/cdns-pltfrm.c
> @@ -328,7 +328,6 @@ static const struct dev_pm_ops cdns_ufs_dev_pm_ops = {
> static struct platform_driver cdns_ufs_pltfrm_driver = {
> .probe = cdns_ufs_pltfrm_probe,
> .remove = cdns_ufs_pltfrm_remove,
> - .shutdown = ufshcd_pltfrm_shutdown,
> .driver = {
> .name = "cdns-ufshcd",
> .pm = &cdns_ufs_dev_pm_ops,
> diff --git a/drivers/ufs/host/tc-dwc-g210-pci.c b/drivers/ufs/host/tc-dwc-g210-pci.c
> index 92b8ad4b58fe..f96fe5855841 100644
> --- a/drivers/ufs/host/tc-dwc-g210-pci.c
> +++ b/drivers/ufs/host/tc-dwc-g210-pci.c
> @@ -32,15 +32,6 @@ static struct ufs_hba_variant_ops tc_dwc_g210_pci_hba_vops = {
> .link_startup_notify = ufshcd_dwc_link_startup_notify,
> };
>
> -/**
> - * tc_dwc_g210_pci_shutdown - main function to put the controller in reset state
> - * @pdev: pointer to PCI device handle
> - */
> -static void tc_dwc_g210_pci_shutdown(struct pci_dev *pdev)
> -{
> - ufshcd_shutdown((struct ufs_hba *)pci_get_drvdata(pdev));
> -}
> -
> /**
> * tc_dwc_g210_pci_remove - de-allocate PCI/SCSI host and host memory space
> * data structure memory
> @@ -137,7 +128,6 @@ static struct pci_driver tc_dwc_g210_pci_driver = {
> .id_table = tc_dwc_g210_pci_tbl,
> .probe = tc_dwc_g210_pci_probe,
> .remove = tc_dwc_g210_pci_remove,
> - .shutdown = tc_dwc_g210_pci_shutdown,
> .driver = {
> .pm = &tc_dwc_g210_pci_pm_ops
> },
> diff --git a/drivers/ufs/host/tc-dwc-g210-pltfrm.c b/drivers/ufs/host/tc-dwc-g210-pltfrm.c
> index f15a84d0c176..4d5389dd9585 100644
> --- a/drivers/ufs/host/tc-dwc-g210-pltfrm.c
> +++ b/drivers/ufs/host/tc-dwc-g210-pltfrm.c
> @@ -92,7 +92,6 @@ static const struct dev_pm_ops tc_dwc_g210_pltfm_pm_ops = {
> static struct platform_driver tc_dwc_g210_pltfm_driver = {
> .probe = tc_dwc_g210_pltfm_probe,
> .remove = tc_dwc_g210_pltfm_remove,
> - .shutdown = ufshcd_pltfrm_shutdown,
> .driver = {
> .name = "tc-dwc-g210-pltfm",
> .pm = &tc_dwc_g210_pltfm_pm_ops,
> diff --git a/drivers/ufs/host/ufs-exynos.c b/drivers/ufs/host/ufs-exynos.c
> index 0bf5390739e1..f41056f57fd7 100644
> --- a/drivers/ufs/host/ufs-exynos.c
> +++ b/drivers/ufs/host/ufs-exynos.c
> @@ -1757,7 +1757,6 @@ static const struct dev_pm_ops exynos_ufs_pm_ops = {
> static struct platform_driver exynos_ufs_pltform = {
> .probe = exynos_ufs_probe,
> .remove = exynos_ufs_remove,
> - .shutdown = ufshcd_pltfrm_shutdown,
> .driver = {
> .name = "exynos-ufshc",
> .pm = &exynos_ufs_pm_ops,
> diff --git a/drivers/ufs/host/ufs-hisi.c b/drivers/ufs/host/ufs-hisi.c
> index 4c423eba8aa9..18b72e2e68c1 100644
> --- a/drivers/ufs/host/ufs-hisi.c
> +++ b/drivers/ufs/host/ufs-hisi.c
> @@ -593,7 +593,6 @@ static const struct dev_pm_ops ufs_hisi_pm_ops = {
> static struct platform_driver ufs_hisi_pltform = {
> .probe = ufs_hisi_probe,
> .remove = ufs_hisi_remove,
> - .shutdown = ufshcd_pltfrm_shutdown,
> .driver = {
> .name = "ufshcd-hisi",
> .pm = &ufs_hisi_pm_ops,
> diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-mediatek.c
> index 73e217260390..e89b625d3c5a 100644
> --- a/drivers/ufs/host/ufs-mediatek.c
> +++ b/drivers/ufs/host/ufs-mediatek.c
> @@ -1650,7 +1650,6 @@ static const struct dev_pm_ops ufs_mtk_pm_ops = {
> static struct platform_driver ufs_mtk_pltform = {
> .probe = ufs_mtk_probe,
> .remove = ufs_mtk_remove,
> - .shutdown = ufshcd_pltfrm_shutdown,
> .driver = {
> .name = "ufshcd-mtk",
> .pm = &ufs_mtk_pm_ops,
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index 82d02e7f3b4f..059de74dfea3 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -1723,7 +1723,6 @@ static const struct dev_pm_ops ufs_qcom_pm_ops = {
> static struct platform_driver ufs_qcom_pltform = {
> .probe = ufs_qcom_probe,
> .remove = ufs_qcom_remove,
> - .shutdown = ufshcd_pltfrm_shutdown,
> .driver = {
> .name = "ufshcd-qcom",
> .pm = &ufs_qcom_pm_ops,
> diff --git a/drivers/ufs/host/ufs-sprd.c b/drivers/ufs/host/ufs-sprd.c
> index 051f3f40d92c..2bad75dd6d58 100644
> --- a/drivers/ufs/host/ufs-sprd.c
> +++ b/drivers/ufs/host/ufs-sprd.c
> @@ -444,7 +444,6 @@ static const struct dev_pm_ops ufs_sprd_pm_ops = {
> static struct platform_driver ufs_sprd_pltform = {
> .probe = ufs_sprd_probe,
> .remove = ufs_sprd_remove,
> - .shutdown = ufshcd_pltfrm_shutdown,
> .driver = {
> .name = "ufshcd-sprd",
> .pm = &ufs_sprd_pm_ops,
> diff --git a/drivers/ufs/host/ufshcd-pci.c b/drivers/ufs/host/ufshcd-pci.c
> index 9c911787f84c..38276dac8e52 100644
> --- a/drivers/ufs/host/ufshcd-pci.c
> +++ b/drivers/ufs/host/ufshcd-pci.c
> @@ -504,15 +504,6 @@ static int ufshcd_pci_restore(struct device *dev)
> }
> #endif
>
> -/**
> - * ufshcd_pci_shutdown - main function to put the controller in reset state
> - * @pdev: pointer to PCI device handle
> - */
> -static void ufshcd_pci_shutdown(struct pci_dev *pdev)
> -{
> - ufshcd_shutdown((struct ufs_hba *)pci_get_drvdata(pdev));
> -}
> -
> /**
> * ufshcd_pci_remove - de-allocate PCI/SCSI host and host memory space
> * data structure memory
> @@ -618,7 +609,6 @@ static struct pci_driver ufshcd_pci_driver = {
> .id_table = ufshcd_pci_tbl,
> .probe = ufshcd_pci_probe,
> .remove = ufshcd_pci_remove,
> - .shutdown = ufshcd_pci_shutdown,
> .driver = {
> .pm = &ufshcd_pci_pm_ops
> },
> diff --git a/drivers/ufs/host/ufshcd-pltfrm.c b/drivers/ufs/host/ufshcd-pltfrm.c
> index 5739ff007828..0b7430033047 100644
> --- a/drivers/ufs/host/ufshcd-pltfrm.c
> +++ b/drivers/ufs/host/ufshcd-pltfrm.c
> @@ -190,12 +190,6 @@ static int ufshcd_parse_regulator_info(struct ufs_hba *hba)
> return err;
> }
>
> -void ufshcd_pltfrm_shutdown(struct platform_device *pdev)
> -{
> - ufshcd_shutdown((struct ufs_hba *)platform_get_drvdata(pdev));
> -}
> -EXPORT_SYMBOL_GPL(ufshcd_pltfrm_shutdown);
> -
> static void ufshcd_init_lanes_per_dir(struct ufs_hba *hba)
> {
> struct device *dev = hba->dev;
> diff --git a/drivers/ufs/host/ufshcd-pltfrm.h b/drivers/ufs/host/ufshcd-pltfrm.h
> index 2e4ba2bfbcad..2df108f4ac13 100644
> --- a/drivers/ufs/host/ufshcd-pltfrm.h
> +++ b/drivers/ufs/host/ufshcd-pltfrm.h
> @@ -31,7 +31,6 @@ int ufshcd_get_pwr_dev_param(const struct ufs_dev_params *dev_param,
> void ufshcd_init_pwr_dev_param(struct ufs_dev_params *dev_param);
> int ufshcd_pltfrm_init(struct platform_device *pdev,
> const struct ufs_hba_variant_ops *vops);
> -void ufshcd_pltfrm_shutdown(struct platform_device *pdev);
> int ufshcd_populate_vreg(struct device *dev, const char *name,
> struct ufs_vreg **out_vreg);
>
> diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
> index 721ae4cd3702..d6da1efb0212 100644
> --- a/include/ufs/ufshcd.h
> +++ b/include/ufs/ufshcd.h
> @@ -1278,7 +1278,6 @@ extern int ufshcd_system_freeze(struct device *dev);
> extern int ufshcd_system_thaw(struct device *dev);
> extern int ufshcd_system_restore(struct device *dev);
> #endif
> -extern int ufshcd_shutdown(struct ufs_hba *hba);
>
> extern int ufshcd_dme_configure_adapt(struct ufs_hba *hba,
> int agreed_gear,
prev parent reply other threads:[~2023-04-26 6:08 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-04-25 23:29 [PATCH 0/4] UFS host controller driver patches Bart Van Assche
2023-04-25 23:29 ` [PATCH 1/4] scsi: ufs: Increase the START STOP UNIT timeout from one to ten seconds Bart Van Assche
2023-04-26 11:39 ` Bean Huo
2023-04-25 23:29 ` [PATCH 2/4] scsi: ufs: Fix handling of lrbp->cmd Bart Van Assche
2023-04-26 12:48 ` Bean Huo
2023-04-26 22:04 ` Bart Van Assche
2023-04-25 23:29 ` [PATCH 3/4] scsi: ufs: Move ufshcd_wl_shutdown() Bart Van Assche
2023-04-25 23:29 ` [PATCH 4/4] scsi: ufs: Simplify driver shutdown Bart Van Assche
2023-04-26 6:07 ` Adrian Hunter [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=91efe449-0000-8e1a-afb8-2628d46dd245@intel.com \
--to=adrian.hunter@intel.com \
--cc=agross@kernel.org \
--cc=alim.akhtar@samsung.com \
--cc=andersson@kernel.org \
--cc=avri.altman@wdc.com \
--cc=baolin.wang@linux.alibaba.com \
--cc=beanhuo@micron.com \
--cc=bvanassche@acm.org \
--cc=dlunev@chromium.org \
--cc=jaegeuk@kernel.org \
--cc=jejb@linux.ibm.com \
--cc=krzysztof.kozlowski@linaro.org \
--cc=linux-scsi@vger.kernel.org \
--cc=mani@kernel.org \
--cc=martin.petersen@oracle.com \
--cc=matthias.bgg@gmail.com \
--cc=orsonzhai@gmail.com \
--cc=quic_asutoshd@quicinc.com \
--cc=quic_cang@quicinc.com \
--cc=stanley.chu@mediatek.com \
--cc=windhl@126.com \
--cc=zhang.lyra@gmail.com \
--cc=zhe.wang1@unisoc.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox