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;
IronPort-SDR: z14ogrvk6FHSNFtjKhzkvK5FT1joKetHqxpSN/YoVdnb1FrIx6X/GJU/a2v42KK2gm3R51kXEl
 OTA8uxbjqNZzusWOvSxjOBxFITHViznwaiKf7V7hy1PJ9GkGnqapAuluXXBNBjS/MHRd8Q38cs
 MoKQ31nd7FFthDZ6Bshkm547+bBKe8va3km/0V7elgdGKDb5xeA0mdvFEZDYVrcFCEpPJBYk/h
 BIrx0UTE8sp5TEvN8qR3++i9QU+mhoUST5l9PgjBWGkPrwVeqn5V8YiSruQlgMzWC3F0Wyud2n
 e34=
From:   Damien Le Moal <Damien.LeMoal@wdc.com>
To:     Vaibhav Gupta <vaibhavgupta40@gmail.com>
CC:     Bjorn Helgaas <helgaas@kernel.org>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Bjorn Helgaas <bjorn@helgaas.com>,
        Vaibhav Gupta <vaibhav.varodek@gmail.com>,
        Jens Axboe <axboe@kernel.dk>,
        Joshua Morris <josh.h.morris@us.ibm.com>,
        Philip Kelleher <pjk1939@linux.ibm.com>,
        "linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-kernel-mentees@lists.linuxfoundation.org" 
        <linux-kernel-mentees@lists.linuxfoundation.org>,
        Shuah Khan <skhan@linuxfoundation.org>
Subject: Re: [PATCH v1 3/3] skd: use generic power management
Thread-Topic: [PATCH v1 3/3] skd: use generic power management
Thread-Index: AQHWXBHK3yyvSpv7dkelMB844h700w==
Date:   Tue, 21 Jul 2020 02:49:06 +0000
Message-ID: <CY4PR04MB3751EB3DA357A6856B3337D0E7780@CY4PR04MB3751.namprd04.prod.outlook.com>
References: <20200717080910.342741-1-vaibhavgupta40@gmail.com>
 <20200717080910.342741-4-vaibhavgupta40@gmail.com>
 <CY4PR04MB37516C5544A7DCAD84921937E77B0@CY4PR04MB3751.namprd04.prod.outlook.com>
 <20200720131627.GA447599@gmail.com>
Accept-Language: en-US
Content-Language: en-US
wdcipoutbound: EOP-TRUE
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 0984d12f-e396-4b19-a57c-08d82d20a2d2
X-MS-Exchange-CrossTenant-originalarrivaltime: 21 Jul 2020 02:49:06.5273
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b61c8803-16f3-4c35-9b17-6f65f441df86
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: UVn5SmPm/6LZftXXOmhdjdfbGImgqvSReTmabn1kw/sJcjaOtIynOIdfDhQQD99Lx25XgVmZdtSgXmMrknnxYw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY4PR04MB0342
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 2020/07/20 22:18, Vaibhav Gupta wrote:=0A=
> On Mon, Jul 20, 2020 at 12:52:14PM +0000, Damien Le Moal wrote:=0A=
>> On 2020/07/17 17:10, Vaibhav Gupta wrote:=0A=
>>> Drivers using legacy PM have to manage PCI states and device's PM state=
s=0A=
>>> themselves. They also need to take care of configuration registers.=0A=
>>>=0A=
>>> With improved and powerful support of generic PM, PCI Core takes care o=
f=0A=
>>> above mentioned, device-independent, jobs.=0A=
>>>=0A=
>>> This driver makes use of PCI helper functions like=0A=
>>> pci_save/restore_state(), pci_enable/disable_device(),=0A=
>>> pci_request/release_regions(), pci_set_power_state() and=0A=
>>> pci_set_master() to do required operations. In generic mode, they are n=
o=0A=
>>> longer needed.=0A=
>>>=0A=
>>> Change function parameter in both .suspend() and .resume() to=0A=
>>> "struct device*" type. Use to_pci_dev() to get "struct pci_dev*" variab=
le.=0A=
>>>=0A=
>>> Compile-tested only.=0A=
>>=0A=
>> I do not think this belongs into the commit message. This was mentioned =
in the=0A=
>> cover letter.=0A=
>>=0A=
> The messages in cover letter and commit message are bit similar. But the =
commit=0A=
> message has patch specific changes mentioned in it.=0A=
=0A=
My point was about the "compile-tested only" mention in the commit message.=
 That=0A=
should be mentioned in the cover letter only. The goal of the patch review=
=0A=
process is also to get these patches tested by others if you do not have ac=
cess=0A=
to the hardware. That is fine, and I can test for the skd driver. But a pat=
ch=0A=
mentioning that it is untested cannot be applied, for obvious reasons :)=0A=
=0A=
So I only requested that you remove the "compiled tested only" note. The co=
mmit=0A=
message could be simpler too, see followup comment.=0A=
=0A=
>>>=0A=
>>> Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com>=0A=
>>> ---=0A=
>>>  drivers/block/skd_main.c | 36 ++++++++++--------------------------=0A=
>>>  1 file changed, 10 insertions(+), 26 deletions(-)=0A=
>>>=0A=
>>> diff --git a/drivers/block/skd_main.c b/drivers/block/skd_main.c=0A=
>>> index 51569c199a6c..d8d1042e7338 100644=0A=
>>> --- a/drivers/block/skd_main.c=0A=
>>> +++ b/drivers/block/skd_main.c=0A=
>>> @@ -3315,12 +3315,12 @@ static void skd_pci_remove(struct pci_dev *pdev=
)=0A=
>>>  	return;=0A=
>>>  }=0A=
>>>  =0A=
>>> -static int skd_pci_suspend(struct pci_dev *pdev, pm_message_t state)=
=0A=
>>> +static int __maybe_unused skd_pci_suspend(struct device *dev)=0A=
>>>  {=0A=
>>>  	int i;=0A=
>>> -	struct skd_device *skdev;=0A=
>>> +	struct pci_dev *pdev =3D to_pci_dev(dev);=0A=
>>> +	struct skd_device *skdev =3D pci_get_drvdata(pdev);=0A=
>>=0A=
>> You can just add the pdev declaration here. The other 2 changes are not =
needed.=0A=
>>=0A=
> Okay.=0A=
>>>  =0A=
>>> -	skdev =3D pci_get_drvdata(pdev);=0A=
>>=0A=
>> You can keep this as is.=0A=
>>=0A=
>>>  	if (!skdev) {=0A=
>>>  		dev_err(&pdev->dev, "no device data for PCI\n");=0A=
>>>  		return -EIO;=0A=
>>> @@ -3337,35 +3337,23 @@ static int skd_pci_suspend(struct pci_dev *pdev=
, pm_message_t state)=0A=
>>>  	if (skdev->pcie_error_reporting_is_enabled)=0A=
>>>  		pci_disable_pcie_error_reporting(pdev);=0A=
>>>  =0A=
>>> -	pci_release_regions(pdev);=0A=
>>> -	pci_save_state(pdev);=0A=
>>> -	pci_disable_device(pdev);=0A=
>>> -	pci_set_power_state(pdev, pci_choose_state(pdev, state));=0A=
>>>  	return 0;=0A=
>>>  }=0A=
>>>  =0A=
>>> -static int skd_pci_resume(struct pci_dev *pdev)=0A=
>>> +static int __maybe_unused skd_pci_resume(struct device *dev)=0A=
>>>  {=0A=
>>>  	int i;=0A=
>>>  	int rc =3D 0;=0A=
>>> -	struct skd_device *skdev;=0A=
>>>  =0A=
>>> -	skdev =3D pci_get_drvdata(pdev);=0A=
>>=0A=
>> Same comment as above. Keep these as is and add only pdev declaration.=
=0A=
>>=0A=
> Okay.=0A=
> Sending v2 with suggested changes.=0A=
> =0A=
> --Vaibhav Gupta=0A=
>>> +	struct pci_dev *pdev =3D to_pci_dev(dev);=0A=
>>> +	struct skd_device *skdev =3D pci_get_drvdata(pdev);=0A=
>>>  	if (!skdev) {=0A=
>>>  		dev_err(&pdev->dev, "no device data for PCI\n");=0A=
>>>  		return -1;=0A=
>>>  	}=0A=
>>>  =0A=
>>> -	pci_set_power_state(pdev, PCI_D0);=0A=
>>> -	pci_enable_wake(pdev, PCI_D0, 0);=0A=
>>> -	pci_restore_state(pdev);=0A=
>>> +	device_wakeup_disable(dev);=0A=
>>>  =0A=
>>> -	rc =3D pci_enable_device(pdev);=0A=
>>> -	if (rc)=0A=
>>> -		return rc;=0A=
>>> -	rc =3D pci_request_regions(pdev, DRV_NAME);=0A=
>>> -	if (rc)=0A=
>>> -		goto err_out;=0A=
>>>  	rc =3D dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));=0A=
>>>  	if (rc)=0A=
>>>  		rc =3D dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));=0A=
>>> @@ -3374,7 +3362,6 @@ static int skd_pci_resume(struct pci_dev *pdev)=
=0A=
>>>  		goto err_out_regions;=0A=
>>>  	}=0A=
>>>  =0A=
>>> -	pci_set_master(pdev);=0A=
>>>  	rc =3D pci_enable_pcie_error_reporting(pdev);=0A=
>>>  	if (rc) {=0A=
>>>  		dev_err(&pdev->dev,=0A=
>>> @@ -3427,10 +3414,6 @@ static int skd_pci_resume(struct pci_dev *pdev)=
=0A=
>>>  		pci_disable_pcie_error_reporting(pdev);=0A=
>>>  =0A=
>>>  err_out_regions:=0A=
>>> -	pci_release_regions(pdev);=0A=
>>> -=0A=
>>> -err_out:=0A=
>>> -	pci_disable_device(pdev);=0A=
>>>  	return rc;=0A=
>>>  }=0A=
>>>  =0A=
>>> @@ -3450,13 +3433,14 @@ static void skd_pci_shutdown(struct pci_dev *pd=
ev)=0A=
>>>  	skd_stop_device(skdev);=0A=
>>>  }=0A=
>>>  =0A=
>>> +static SIMPLE_DEV_PM_OPS(skd_pci_pm_ops, skd_pci_suspend, skd_pci_resu=
me);=0A=
>>> +=0A=
>>>  static struct pci_driver skd_driver =3D {=0A=
>>>  	.name		=3D DRV_NAME,=0A=
>>>  	.id_table	=3D skd_pci_tbl,=0A=
>>>  	.probe		=3D skd_pci_probe,=0A=
>>>  	.remove		=3D skd_pci_remove,=0A=
>>> -	.suspend	=3D skd_pci_suspend,=0A=
>>> -	.resume		=3D skd_pci_resume,=0A=
>>> +	.driver.pm	=3D &skd_pci_pm_ops,=0A=
>>>  	.shutdown	=3D skd_pci_shutdown,=0A=
>>>  };=0A=
>>>  =0A=
>>>=0A=
>>=0A=
>>=0A=
>> -- =0A=
>> Damien Le Moal=0A=
>> Western Digital Research=0A=
> =0A=
=0A=
=0A=
-- =0A=
Damien Le Moal=0A=
Western Digital Research=0A=