Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <f6e7bd7a-c901-a38c-e427-e9671dfb6d6c@intel.com>
Date:   Mon, 28 Nov 2022 13:51:24 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.5.0
Subject: Re: [PATCH V3 1/2] PCI/DOE: Remove the pci_doe_flush_mb() call
To:     <ira.weiny@intel.com>
CC:     Lukas Wunner <lukas@wunner.de>,
        Jonathan Cameron <Jonathan.Cameron@Huawei.com>,
        Alison Schofield <alison.schofield@intel.com>,
        Vishal Verma <vishal.l.verma@intel.com>,
        Gregory Price <gregory.price@memverge.com>,
        <linux-pci@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <linux-cxl@vger.kernel.org>,
        Dan Williams <dan.j.williams@intel.com>,
        Bjorn Helgaas <bhelgaas@google.com>
References: <20221128040338.1936529-1-ira.weiny@intel.com>
 <20221128040338.1936529-2-ira.weiny@intel.com>
Content-Language: en-US
From:   "Li, Ming" <ming4.li@intel.com>
Organization: Intel
In-Reply-To: <20221128040338.1936529-2-ira.weiny@intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?ejVMdjVoWHVvVjJ0R1dSNlNzTTRBV0dtZ05SdkJFeDhTcitIVHpDSXI4M2pu?=
 =?utf-8?B?Sk53aGkwM3M2QkhaZlB2TkE4VG9ORTd1MDhDUWZsWkY5cGpmK202bTdOVEND?=
 =?utf-8?B?VDZCbWRFaFd5WlRQY3pJTTJ2Ly9rYjB6OEcrdng0Vmt2WXIzRmhpTGkwY1Yv?=
 =?utf-8?B?VGFtNk5DbDE3ZnM3VWIzSXY1SE1yVVprS2c3TGtucEFsbUN4SnBZZFhKWVVv?=
 =?utf-8?B?VUN6WG4rdHFKNjlZUURwM29SR3F5eUZ3VHBzSjJOQmVtZ2JreE9neDlOQXZ6?=
 =?utf-8?B?Rlk3bDdBMzBsOGRIWTltaHFaK0dwODFxSWwySUtqUytXWWtJYmVjMGZEZ251?=
 =?utf-8?B?YnFzK3NzQTZzUjhmcHdPcDNaQjdzdUI0aSs0V093UmtlZ2t1OFZyRlAwQVhm?=
 =?utf-8?B?Mlo2OS9maG14Vm1NM21ScmdjR0xDS3lncC9YVlhIZEt2Sk1JUVZrU2paK3V3?=
 =?utf-8?B?ZXdDdWRaaitpU1FlTzlNSzZzdkI0SUNpMVNCRisrVFk0RkpKVjZpY1MrbVlr?=
 =?utf-8?B?NFNHYnhwL0wxdFdhdkI2d1BqcFVuVitGakM5SUJGdVF3K29ITEp0V1c5d3Zy?=
 =?utf-8?B?VjlRcnUvZ0dnVjVCUWZJWTRxT1Nub3IzQTdVeHR4WmJaSmwxZklLOW0yV2hP?=
 =?utf-8?B?RkFma2VaYWtvaW4ydmhHSUpLalEwNlRsa2FFMU1wWlJueEMwa013ZldneWNE?=
 =?utf-8?B?Q2orU1ZSaHZJZzdmVi9PVWtFTjdpR3B3ZjJnQVd1OXBaVGxNSnlGazNicFFB?=
 =?utf-8?B?WHJpck51Z0pKZlBFb0o5RUt0MjVlQVk3QVlwMWRLeldMdmNXdklNQWx4NjBW?=
 =?utf-8?B?dmZFdkFFVDZWS1JjMnF3Uk1vbWhNRUZZaTRUSEhNRzZ6YTVRQVBqbmRHeFNz?=
 =?utf-8?B?Wm94SSt4ZHVjY3B5UnV1WURZOGNLMVhKcVlYT1BRSnZSaE1rSjBoeGViazFI?=
 =?utf-8?B?MEpSWStoUXN1RHArSWFGNDdLV2ZuTjQwbmx1RGFZdXpHMGNNb05UNUhJeGhK?=
 =?utf-8?B?K1lpUzIxNFlZWkwyNmtxZEtJUEQzSEhlY3J5YWJPd2krUmJNVmZJeEo4cDhQ?=
 =?utf-8?B?azVyMCs0QVIvZlo4ZDk4NysvL0lzTDZhL0pBNFozV3NTbSthWW9XYkxaSUZS?=
 =?utf-8?B?SkNSaGpOd0JnNzV4eEFrbUM2RFJieUViUTFPejg3eE5TZTltNVU0bklzOGVS?=
 =?utf-8?B?aEY1OUJJa3QwdStFQ3NycDJZTUtFalA2SVRFaUhlQUxoMElaZ3F5Y1dpQ1Fi?=
 =?utf-8?B?cEFUTW8vMzlyTi96R3BOWmdyZDlFbjRCd0VnRzdtSmRTa0pEKzlQM01xL2pm?=
 =?utf-8?B?UU9iZlBWbnA2YTV3R3ArcHZOUExLZElzUkcyWkVmUi9JWVVCdVMzSnh3ekQv?=
 =?utf-8?B?TitvWG42U2EyQTlNUkpUbmFJMHIveVg4cWhFelNJZzBha3B1UnlmVURzcDNB?=
 =?utf-8?B?R3o2UEZrYm9DZ1lEbU1od0lYdk05S2g0OVoxdUtlMllpd0pXay9SNUhvOGFy?=
 =?utf-8?B?RXJCY0RucXFpU2UrZ1IxSGJPYWwxNThlNjd1YTh6MHJ6cUdXTjgwUmxrVHJY?=
 =?utf-8?B?TUthQjlJTCt2ZVJrZkNxbFlBVmV3b0xvQjd4TWZJNEY5dmZZRjhBSmxSNm9V?=
 =?utf-8?B?dzRPcmtWMVNFTzJkbktCU2Yrenhoa3hlZEhaVnFFN2FxaStiQng0T2NUMlFu?=
 =?utf-8?B?cDlsVXI1UjFLcTMxZ1NRL3ZaT3A5QUpicFRPTHVscmt4aFU5NGxTVUdMdGQ5?=
 =?utf-8?B?SGRUWlJyc055NDBtY0pDckYzK3A4UFl0WEdHOUxjVFBpUVN3akZBbHlRT0pX?=
 =?utf-8?B?VkZQSkFWWlZXTEgzNzVJR0VFWlpEU0h5WXdSWm9vektMbnN5Y2xIQ3lPV2c2?=
 =?utf-8?B?Q1Q1cFVsR3pKQ2xOdzNMeFplcSt4OE9Ob3ZlV2hCYkJYRTJPK0p3SFpUMGJC?=
 =?utf-8?B?MDFqQnFFelkrWk5tN2p4eHViV2NYUm9zdFZaS2tBQXBicWNkd1Via3BIYnJl?=
 =?utf-8?B?ejdxOVR3UzAyUXpzYlo4WXR3dkRtUjVPUWwzRGwwN2FOZUplQWgzTWE2Rnl2?=
 =?utf-8?B?b2R3QitLUFJCM0VVZzd5dHEveU8ra2ZUWUdsZzRTRlQ3ZjJaTGtVc2s1RTlp?=
 =?utf-8?Q?2/QpUb3bagC/TKDNK+55LaCS8?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 0b721db1-d4ad-4821-c570-08dad1049c60
X-MS-Exchange-CrossTenant-AuthSource: BN6PR11MB3956.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 Nov 2022 05:51:36.3602
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: rnKkFsj3hKJT+mHr+isjeP4ZqAY0dlGABVdkKvc98iI5ODQon1KT/2HhSoN1mJRAxmbxPnfu8r3TNkbFysS1NQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO1PR11MB4787
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-7.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_HI,SPF_HELO_NONE,SPF_NONE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 11/28/2022 12:03 PM, ira.weiny@intel.com wrote:
> From: Ira Weiny <ira.weiny@intel.com>
> 
> pci_doe_flush_mb() does not work and is currently unused.
> 
> It does not work because each struct doe_mb is managed as part of the
> PCI device.  They can't go away as long as the PCI device exists.
> pci_doe_flush_mb() was set up to flush the workqueue and prevent any
> further submissions to the mailboxes when the PCI device goes away.
> Unfortunately, this was fundamentally flawed.  There was no guarantee
> that a struct doe_mb remained after pci_doe_flush_mb() returned.
> Therefore, the doe_mb state could be invalid when those threads waiting
> on the workqueue were flushed.
> 
> Fortunately the current code is safe because all callers make a
> synchronous call to pci_doe_submit_task() and maintain a reference on
> the PCI device.  Therefore pci_doe_flush_mb() is effectively unused.
> 
> Rather than attempt to fix pci_doe_flush_mb() just remove the dead code
> around pci_doe_flush_mb().
> 
> Cc: Lukas Wunner <lukas@wunner.de>
> Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>

Some comments inline.

> 
> ---
> Changes from V2:
> 	Lukas
> 		Clarify commit message.
> 	Jonathan
> 		Add comment for changed poll interval.

...

>  
> -static int pci_doe_wait(struct pci_doe_mb *doe_mb, unsigned long timeout)
> -{
> -	if (wait_event_timeout(doe_mb->wq,
> -			       test_bit(PCI_DOE_FLAG_CANCEL, &doe_mb->flags),
> -			       timeout))
> -		return -EIO;
> -	return 0;
> -}
> -
>  static void pci_doe_write_ctrl(struct pci_doe_mb *doe_mb, u32 val)
>  {
>  	struct pci_dev *pdev = doe_mb->pdev;
> @@ -82,12 +73,9 @@ static int pci_doe_abort(struct pci_doe_mb *doe_mb)
>  	pci_doe_write_ctrl(doe_mb, PCI_DOE_CTRL_ABORT);
>  
>  	do {
> -		int rc;
>  		u32 val;
>  
> -		rc = pci_doe_wait(doe_mb, PCI_DOE_POLL_INTERVAL);
> -		if (rc)
> -			return rc;
> +		msleep_interruptible(PCI_DOE_POLL_INTERVAL_MSECS);
>  		pci_read_config_dword(pdev, offset + PCI_DOE_STATUS, &val);

Looks like we don't have to use msleep_interruptible() here, can use msleep() directly?

>  
>  		/* Abort success! */
> @@ -278,11 +266,7 @@ static void doe_statemachine_work(struct work_struct *work)
>  			signal_task_abort(task, -EIO);
>  			return;
>  		}
> -		rc = pci_doe_wait(doe_mb, PCI_DOE_POLL_INTERVAL);
> -		if (rc) {
> -			signal_task_abort(task, rc);
> -			return;
> -		}
> +		msleep_interruptible(PCI_DOE_POLL_INTERVAL_MSECS);
>  		goto retry_resp;
>  	}

I guess that you use msleep_interruptible() here for aborting current task when signals come.
So there should be signal_task_abort() and return when msleep_interruptible() receives a signal.

Thanks
Ming

>  
> @@ -383,21 +367,6 @@ static void pci_doe_destroy_workqueue(void *mb)
>  	destroy_workqueue(doe_mb->work_queue);
>  }
>  
> -static void pci_doe_flush_mb(void *mb)
> -{
> -	struct pci_doe_mb *doe_mb = mb;
> -
> -	/* Stop all pending work items from starting */
> -	set_bit(PCI_DOE_FLAG_DEAD, &doe_mb->flags);
> -
> -	/* Cancel an in progress work item, if necessary */
> -	set_bit(PCI_DOE_FLAG_CANCEL, &doe_mb->flags);
> -	wake_up(&doe_mb->wq);
> -
> -	/* Flush all work items */
> -	flush_workqueue(doe_mb->work_queue);
> -}
> -
>  /**
>   * pcim_doe_create_mb() - Create a DOE mailbox object
>   *
> @@ -450,14 +419,6 @@ struct pci_doe_mb *pcim_doe_create_mb(struct pci_dev *pdev, u16 cap_offset)
>  		return ERR_PTR(rc);
>  	}
>  
> -	/*
> -	 * The state machine and the mailbox should be in sync now;
> -	 * Set up mailbox flush prior to using the mailbox to query protocols.
> -	 */
> -	rc = devm_add_action_or_reset(dev, pci_doe_flush_mb, doe_mb);
> -	if (rc)
> -		return ERR_PTR(rc);
> -
>  	rc = pci_doe_cache_protocols(doe_mb);
>  	if (rc) {
>  		pci_err(pdev, "[%x] failed to cache protocols : %d\n",