Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v4 01/10] scsi: ufs: Rename flags pm_op_in_progress and
 is_sys_suspended
From:   Bart Van Assche <bvanassche@acm.org>
To:     Can Guo <cang@codeaurora.org>, asutoshd@codeaurora.org,
        nguyenb@codeaurora.org, hongwus@codeaurora.org,
        ziqichen@codeaurora.org, linux-scsi@vger.kernel.org,
        kernel-team@android.com
Cc:     Andy Gross <agross@kernel.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Alim Akhtar <alim.akhtar@samsung.com>,
        Avri Altman <avri.altman@wdc.com>,
        "James E.J. Bottomley" <jejb@linux.ibm.com>,
        "Martin K. Petersen" <martin.petersen@oracle.com>,
        Stanley Chu <stanley.chu@mediatek.com>,
        Bean Huo <beanhuo@micron.com>,
        Jaegeuk Kim <jaegeuk@kernel.org>,
        Adrian Hunter <adrian.hunter@intel.com>,
        Kiwoong Kim <kwmad.kim@samsung.com>,
        Satya Tangirala <satyat@google.com>,
        "open list:ARM/QUALCOMM SUPPORT" <linux-arm-msm@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>
References: <1624433711-9339-1-git-send-email-cang@codeaurora.org>
 <1624433711-9339-2-git-send-email-cang@codeaurora.org>
 <1c5db457-ee87-2308-15f5-5dad49508f10@acm.org>
Message-ID: <c7c5d95e-8a69-dfef-44ea-bfcadd6ea5d5@acm.org>
Date:   Wed, 23 Jun 2021 13:57:05 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.11.0
MIME-Version: 1.0
In-Reply-To: <1c5db457-ee87-2308-15f5-5dad49508f10@acm.org>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 6/23/21 1:05 PM, Bart Van Assche wrote:
> On 6/23/21 12:35 AM, Can Guo wrote:
>> Rename pm_op_in_progress and is_sys_suspended to wlu_pm_op_in_progress and
>> is_wlu_sys_suspended accordingly.
> 
> My understanding is that power management operations must be submitted
> to one particular UFS WLUN (hba->sdev_ufs_device). That makes the "wlu_"
> part of the new names redundant. In other words, I like the current
> names better than the new names. Unless if I missed something, consider
> dropping this patch.

Hi Can,

Reviewing later patches in this series made me realize that there are
two families of suspend/resume functions. One family of functions
operates at the platform level while the other family operates at the
SCSI LUN level. My comments about the suspend/resume functions are as
follows:
- It seems redundant to me to have system suspend support at the SCSI
  LUN level (__ufshcd_wl_suspend(hba, UFS_SYSTEM_PM)) and also at the
  platform level. Since the platform device is a parent of the SCSI
  WLUN, can system suspend/resume support be left out from
  ufshcd_wl_pm_ops (or in other words, remove the .freeze and .thaw
  callbacks)? Do we really need two calls from the power management
  subsystem into the UFS driver for every system suspend and every
  system resume?
- Because of the device links (device_link_add()), the ufschd_wl_*()
  RPM callbacks are invoked after all LUNs have been suspended. I would
  appreciate it if the "ufshcd_wl_" prefix would be changed into
  "ufshcd_lun_" since that would make it more clear that these callbacks
  are associated with all LUNs and not only with the WLUN through which
  power management commands are submitted.

Thanks,

Bart.