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;
Date:   Mon, 21 Nov 2022 14:59:46 -0800
From:   Ira Weiny <ira.weiny@intel.com>
To:     "Li, Ming" <ming4.li@intel.com>
CC:     Gregory Price <gregory.price@memverge.com>,
        Jonathan Cameron <Jonathan.Cameron@huawei.com>,
        Vishal Verma <vishal.l.verma@intel.com>,
        "Lukas Wunner" <lukas@wunner.de>,
        Alison Schofield <alison.schofield@intel.com>,
        <linux-cxl@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <linux-pci@vger.kernel.org>,
        Dan Williams <dan.j.williams@intel.com>,
        "Bjorn Helgaas" <bhelgaas@google.com>
Subject: Re: [PATCH] PCI/DOE: Remove asynchronous task support
Message-ID: <Y3wC4kX6SCr90FGY@iweiny-desk3>
References: <20221119222527.1799836-1-ira.weiny@intel.com>
 <750c2096-fe2f-0597-a635-78cbe6b0ee0d@intel.com>
 <e7db6aea-4146-33f2-9490-9b5b902e0ec1@intel.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <e7db6aea-4146-33f2-9490-9b5b902e0ec1@intel.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?il0t3IUUji2A81OkEWFiPJZEWoAN7XcUCZc0h5JUxTMDfBC6q9opkR9CR/3a?=
 =?us-ascii?Q?iBGXnW2OTG2s9uB3EhIdJrU4rhccq4L9FeW/yGYiivXpz0hafRhuKyM6OE/F?=
 =?us-ascii?Q?qa0NY+Qmegf0WdBh4yXVd8jQ0LtvOOIabmLrGQrU4fn989adOA/ZxFKa5hqd?=
 =?us-ascii?Q?C9SX3RROWQOHJuG4b+dyYjifDgMDA/fCYp72/8JQuTxw18KjRFg73jZ8K+Np?=
 =?us-ascii?Q?TVVrA3r4xzdKUieKxsT4cAaIl/+0J4NLtRMEtaDHM5sMOutN0T0NZSISH7Y0?=
 =?us-ascii?Q?LfaAB7uePuj4OaXFqq2SAldJwSpRbIIQ+qiOQ+2egsjOoutS+JhNucT3UHlC?=
 =?us-ascii?Q?qig4KrOm4fgc2EWjaPotYL9XAy/O9p8WdndJ2zAIwTFYLFmwXNFCeP6126h/?=
 =?us-ascii?Q?cbLvtLpVg7dPy55l9p2vswlsYzRewaSvxhlza9yJ4cUrIPkQo1M/ArIdBUEV?=
 =?us-ascii?Q?CMQCJNB99xMX5Sdf9eaGAKpoj9+bQgK8H9ATSiLIzir+nZ0dXBzRxjcAvscV?=
 =?us-ascii?Q?s/Ysxq2UOx6PCACBebVQYC2UU81rPBwjBOZLZGX7cO/y59+Dbo84QpiStGpL?=
 =?us-ascii?Q?8LBPl4+KIPOcgezaVzKBJ5eTKI6D4W//YSOsjoHO4FppCxeX5k4iHwshhZ1y?=
 =?us-ascii?Q?e57e3vENAn/lOepNb2SAE8f9fMKcoZF/K9xThlBvmBbFCXwFvceppakevKUB?=
 =?us-ascii?Q?z5J1ugPcihSKwEWuqd4ISkdA5ND5dQ1L/YfXcGp85grzrxZ26fEVRXQ4t7ME?=
 =?us-ascii?Q?jb5YI3IRAQ2Th0eJKysDT/0hjERFbLbmqpvRh0zB+zb4tNOwvZsNxdNQlNXO?=
 =?us-ascii?Q?OqBrdLcoCH29CUmFhdQHuCb04ClyAL7z9vaWNhfG5lRMPtwctkJckSlEiwCY?=
 =?us-ascii?Q?APjgQHcRzP40DWYlIbFFcyI1znUnVMQ5Wc0Pp0FDOtiWxiLiX2Eg60JyjL8e?=
 =?us-ascii?Q?FYwG39X1so7BJdHJtCytE2u1htUeaX0MjmWaDVq17PMYpfRIVkEcwLpvpd2A?=
 =?us-ascii?Q?SY7Aijjns9fpj5Ir2fd1giG6BQpH1f9wKl21tqbl/ybDZKFTmBFmJFJ9wsbb?=
 =?us-ascii?Q?KyxnUrP1+vqO4L2xVRDUQ4IyudmtFaTxs+xQi/M3SiicJyMDsLDlABKc51rA?=
 =?us-ascii?Q?mUBzAYRwqP9f3YGF/sd40WLCuC3ErmSgDxfFwEVYXV8yCVBejppMg8+Qxv2Q?=
 =?us-ascii?Q?P0L2cw6C1gX8Wy+SGsrLjeStygXbmOA4nLb9uMNAPzbE7WIv3Jfa10mbtvV5?=
 =?us-ascii?Q?gR+CxjtCcIF7UFY7zgSdgypgpiQcsEOzIp5BhjWJ+nGC6UEy/rNIFcPW0s8X?=
 =?us-ascii?Q?gDgzsbstxNgZJY88lPRrP6A8ZEoK8NroIB9lBCZQpTIHQXaQLFGdXBSWBzs2?=
 =?us-ascii?Q?ochOstWqU3J3CwxyL3SP6qJfAde6rmPsixmFhqqJE5gYw8eEXG3xOnxjs1CQ?=
 =?us-ascii?Q?yN6UpO/62cR0KS1at+AF2t3bG2Ifsx8um+GH6Py0S0v0OOONwT7qPGpxLfp7?=
 =?us-ascii?Q?ggk4kzYxoWRTdhsCSSTxnFdvJuimtuwIhoPN7gWD0zS9xWd2VUaH5faNHzTG?=
 =?us-ascii?Q?RiicjlXlaDlydnx4Q1sWJnej6dDQMq/a22UCCJsP?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 2b9ce800-be38-47a5-a22d-08dacc14187a
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Nov 2022 22:59:50.9855
 (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: A+UGrI/XbZnNk1VsbrQ59vXZLA3LWwI8/g9C0Ppfa2hktVZcSnTK2g+aYo4Ao6lTTeFgXTJU+V2C8cHelrEEqA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR11MB6520
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        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 Mon, Nov 21, 2022 at 10:07:56AM +0800, Li, Ming wrote:
> On 11/21/2022 9:39 AM, Li, Ming wrote:

[snip]

> >> @@ -529,8 +492,18 @@ int pci_doe_submit_task(struct pci_doe_mb *doe_mb, struct pci_doe_task *task)
> >>  		return -EIO;
> >>  
> >>  	task->doe_mb = doe_mb;
> >> -	INIT_WORK(&task->work, doe_statemachine_work);
> >> -	queue_work(doe_mb->work_queue, &task->work);
> >> +
> >> +again:
> >> +	if (!mutex_trylock(&doe_mb->exec_lock)) {
> >> +		if (wait_event_timeout(task->doe_mb->wq,
> >> +				test_bit(PCI_DOE_FLAG_CANCEL, &doe_mb->flags),
> >> +				PCI_DOE_POLL_INTERVAL))
> >> +			return -EIO;
> > 
> > We already implemented a pci_doe_wait(), I think we can use it to instead of this wait_event_timeout.
> > 
> > Thanks
> > Ming
> > 
> 
> This wait_event_timeout() only check PCI_DOE_FLAG_CANCEL, that means it only detects the signal which the doe_mb has being destroyed.
> If current doe task is done correctly, I think we should wake up next task. Current implementation just waits utill timeout happens and try it again.
> Besides, If two threads are waiting a same doe_mb, thread #1 waited firstly, thread #2 waited secondly, there is a chance that thread #2 is processed before thread #1.
> 

Agreed.

However, the real problem is that the doe_mb is probably free'ed at this point
and all this is going to crash and burn anyway.  The implementation of
PCI_DOE_FLAG_CANCEL was fundamentally flawed even for the current work queue
implementation.

This patch incorrectly tried to use that mechanism but upon looking closer I
see it does not work.

I saw in another thread Jonathan discussing some sort of get/put on the doe_mb.
That is not currently necessary as the creators of doe_mb objects currently
hold references to the PCI device any time they call submit.

:-(

For now all PCI_DOE_FLAG_CANCEL stuff needs to go away,
Ira

> Thanks
> Ming
> 
> >> +		goto again;
> >> +	}
> >> +	exec_task(task);
> >> +	mutex_unlock(&doe_mb->exec_lock);
> >> +
> >>  	return 0;
> >>  }
> >> -EXPORT_SYMBOL_GPL(pci_doe_submit_task);
> >> +EXPORT_SYMBOL_GPL(pci_doe_submit_task_wait);
> >> diff --git a/include/linux/pci-doe.h b/include/linux/pci-doe.h
> >> index ed9b4df792b8..c94122a66221 100644
> >> --- a/include/linux/pci-doe.h
> >> +++ b/include/linux/pci-doe.h
> >> @@ -30,8 +30,6 @@ struct pci_doe_mb;
> >>   * @response_pl_sz: Size of the response payload (bytes)
> >>   * @rv: Return value.  Length of received response or error (bytes)
> >>   * @complete: Called when task is complete
> >> - * @private: Private data for the consumer
> >> - * @work: Used internally by the mailbox
> >>   * @doe_mb: Used internally by the mailbox
> >>   *
> >>   * The payload sizes and rv are specified in bytes with the following
> >> @@ -50,11 +48,6 @@ struct pci_doe_task {
> >>  	u32 *response_pl;
> >>  	size_t response_pl_sz;
> >>  	int rv;
> >> -	void (*complete)(struct pci_doe_task *task);
> >> -	void *private;
> >> -
> >> -	/* No need for the user to initialize these fields */
> >> -	struct work_struct work;
> >>  	struct pci_doe_mb *doe_mb;
> >>  };
> >>  
> >> @@ -72,6 +65,5 @@ struct pci_doe_task {
> >>  
> >>  struct pci_doe_mb *pcim_doe_create_mb(struct pci_dev *pdev, u16 cap_offset);
> >>  bool pci_doe_supports_prot(struct pci_doe_mb *doe_mb, u16 vid, u8 type);
> >> -int pci_doe_submit_task(struct pci_doe_mb *doe_mb, struct pci_doe_task *task);
> >> -
> >> +int pci_doe_submit_task_wait(struct pci_doe_mb *doe_mb, struct pci_doe_task *task);
> >>  #endif
> >>
> >> base-commit: b6e7fdfd6f6a8bf88fcdb4a45da52c42ba238c25