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:   Thu, 23 Jun 2022 11:07:58 -0700
From:   Dan Williams <dan.j.williams@intel.com>
To:     Ira Weiny <ira.weiny@intel.com>,
        Dan Williams <dan.j.williams@intel.com>
CC:     Bjorn Helgaas <bhelgaas@google.com>,
        Jonathan Cameron <Jonathan.Cameron@huawei.com>,
        Alison Schofield <alison.schofield@intel.com>,
        Vishal Verma <vishal.l.verma@intel.com>,
        Dave Jiang <dave.jiang@intel.com>,
        Ben Widawsky <bwidawsk@kernel.org>,
        <linux-kernel@vger.kernel.org>, <linux-cxl@vger.kernel.org>,
        <linux-pci@vger.kernel.org>
Subject: Re: [PATCH V11 3/8] PCI: Create PCI library functions in support of
 DOE mailboxes.
Message-ID: <62b4abfe54d0c_3c3ff029484@dwillia2-xfh.notmuch>
References: <20220610202259.3544623-1-ira.weiny@intel.com>
 <20220610202259.3544623-4-ira.weiny@intel.com>
 <62ad06a643ddf_844b1294f@dwillia2-xfh.notmuch>
 <YrObe+AdfziEPlYB@iweiny-desk3>
 <62b39e5eead1_32f38a294b6@dwillia2-xfh.notmuch>
 <YrOy3vodu3FcRjUY@iweiny-desk3>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <YrOy3vodu3FcRjUY@iweiny-desk3>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?b5DZNZvVi4LRj9Q3pGsUV0OEca4dPiIVKj3rrN2Tv7/exFBFlirQkbVKwumw?=
 =?us-ascii?Q?EpW7Py5YGZ9Gt+PdwRWJTWKgNuneyJ59MVH54jmUXbP4eKZ10TL5zd8ehHm3?=
 =?us-ascii?Q?J6SHsUvk9h3IE8R4sSg3j+fFcTnfpWMEDgPjyG97uySbLl/L1UBLmt5qzIJP?=
 =?us-ascii?Q?wItna8akAE70mba98854VSBEt341tys7+baZn81mnsq52DQR7KDTO1dt35ha?=
 =?us-ascii?Q?P7sNyVjUFL13F5vthkPgTfvAp+9AVUOrsLRNZn3A/lpJRpkdHXu0q4GYQCgD?=
 =?us-ascii?Q?K9EaLAdCowTIrOQ8vPWskj076t61anVoW9YO4dj3V8fvELRBOWtG+ksESetZ?=
 =?us-ascii?Q?+dIbIlyaPOMMfagoukI/92m57BSFif1IAj/qGNSx0SEhxmVxyNlSmx3zlLBk?=
 =?us-ascii?Q?W2Yf2hwPbXu8vFS+hwAqUHRM1+7eiSjfPPAtAsjKSgOtsVpw+alFaqugYFCX?=
 =?us-ascii?Q?1ONqYKyAmNFi6p4TPQQuoyNV+AbcpzeFnsuW042pfMRCfJKcr0MtW2CVlvzh?=
 =?us-ascii?Q?dpsYrMva7peLMpd5Z5zt/k25kX6A2BShCNhUm3LyRnxvOYVCcwaW0F5uI6aP?=
 =?us-ascii?Q?Zsvqz6L1wT4PNHpONKPGspOhoOapSuml9a19iGZp2LhLe+afpQlc4CMfez/p?=
 =?us-ascii?Q?lBMy3DUpEsXxiTp/IjkPtEylBvsvSdRl4ViAbZ9nPvVLVqWvUCV4mlaMUqU0?=
 =?us-ascii?Q?mT0oaeDqYo66zIbIVpbXqLLkJPSm9MyAXz+7TiB7WgXWyy305H9YWxxuSiMH?=
 =?us-ascii?Q?lsglQ9WjsH349H4vH9Wp0nh59rHwBExFMfw5Gk8dKZN4nUA8LsikZzz0OyKN?=
 =?us-ascii?Q?JlsZZWxijQIie+zD+YxvRgcWKFSKmA8wdPvVhIPWyvXS+O8pi3Daj7EIH4rN?=
 =?us-ascii?Q?QyCJNf6bHdiWE3h0J8wrjxYRoqk1DZvFX6vdhyC56ejTcm4108UzF3V2Bn7X?=
 =?us-ascii?Q?bNWvtN/LMBdmSfLCblq7OC4SSaN6qEC4NObU4W+2cuU+gtiWLRewDyKxcdtA?=
 =?us-ascii?Q?DMgxF57gtR5Q26vnHuykaY4vCOAFrhMhPQeFZp/Svmts4wvAtw7VW/0Mdp1k?=
 =?us-ascii?Q?uJfSpXO7h3bBPMopnYHTMQkTzcipNNWsgOlFwpwdfF1hjkB3IgXW2sjb3x4g?=
 =?us-ascii?Q?X6BeUNcfw6AanCmCDvuQWg8W5L7RlpjT4hdyljIhNyallbis2RMoDur1Dvef?=
 =?us-ascii?Q?QCAfrB12tmLPkEdQ9RNafGtdNyrrGhYkCRsDLYV2aHJ/ZvP0HiiLcnb7cO/P?=
 =?us-ascii?Q?093Q4tp+ArJ0rotB1VpOZTQb0dE02kbCQz3LqmNEYg8P4wf05MkF+DOD7u4y?=
 =?us-ascii?Q?39+oM02dZ9rS6w7/inGPm2521kbkfxfZzUkTR1/htnxB6u5MkDdNO8GJ1VAt?=
 =?us-ascii?Q?m9Vdh0tC5M9MhXLwt67wKLNwP74+yt+wo6BIj1xL22h4uv96ptVhwyH1gfAD?=
 =?us-ascii?Q?k9sQbtxr7oxXu+YoNCR229bHywpoGBUk0Yc9CPc7K5OnGa8EPbeFbMumTM0b?=
 =?us-ascii?Q?VTxHJesV4gUyDRjZpFGr/2lpajbNzYReclrl+ExKt2ISYQZX2IQwbVj/6SoZ?=
 =?us-ascii?Q?jfnk2GSqgvEK9KtkxxDlACUXb6J7SvciCfOmuLAgzYzEqavSWlptwlmXJ9tB?=
 =?us-ascii?Q?WqNw10HNdNWYlUzzLd7FPGp4A+jR1RiuEKtFZIStQPZTO6H6/AwGqZ851Qil?=
 =?us-ascii?Q?DZqeNQkp4tgPPV3zJKt8lVo0H2bXlDP1G2auMQRcSajKOG4FX37i1qoxanF0?=
 =?us-ascii?Q?DuDEmSygNvLyvlvKuBVAQXblJijPj6w=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: e2005d16-4c5d-4554-ad38-08da55434ee4
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1101MB2126.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Jun 2022 18:08:00.3625
 (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: qhwKv+q4rmTBWb9CHgUG3jA8B/bLQyaufjOovTdhKaLq0zSq8WFljb/OFqlAGulz1nZe/zi0+eCrWXCOvbYv/RElGwB3tlYusKGGwE28r+A=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR11MB4612
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-5.0 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,T_SCC_BODY_TEXT_LINE 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

Ira Weiny wrote:
> On Wed, Jun 22, 2022 at 03:57:34PM -0700, Dan Williams wrote:
> > Ira Weiny wrote:
> > > On Fri, Jun 17, 2022 at 03:56:38PM -0700, Dan Williams wrote:
> > [..]
> > > > > +static int pci_doe_discovery(struct pci_doe_mb *doe_mb, u8 *index, u16 *vid,
> > > > > +			     u8 *protocol)
> > > > > +{
> > > > > +	u32 request_pl = FIELD_PREP(PCI_DOE_DATA_OBJECT_DISC_REQ_3_INDEX,
> > > > > +				    *index);
> > > > > +	u32 response_pl;
> > > > > +	DECLARE_COMPLETION_ONSTACK(c);
> > > > > +	struct pci_doe_task task = {
> > > > > +		.prot.vid = PCI_VENDOR_ID_PCI_SIG,
> > > > > +		.prot.type = PCI_DOE_PROTOCOL_DISCOVERY,
> > > > > +		.request_pl = &request_pl,
> > > > > +		.request_pl_sz = sizeof(request_pl),
> > > > > +		.response_pl = &response_pl,
> > > > > +		.response_pl_sz = sizeof(response_pl),
> > > > > +		.complete = pci_doe_task_complete,
> > > > > +		.private = &c,
> > > > > +	};
> > > > > +	int ret;
> > > > > +
> > > > > +	ret = pci_doe_submit_task(doe_mb, &task);
> > > > > +	if (ret < 0)
> > > > > +		return ret;
> > > > > +
> > > > > +	wait_for_completion(&c);
> > > > 
> > > > Another place where the need for a completion can be replaced with
> > > > flush_work().
> > > 
> > > No not here.  While this call is internal it is actually acting like an
> > > external caller.  This specific wait is for that response to get back.
> > > 
> > > This pattern was specifically asked for by you.  Previously Jonathan had a
> > > synchronous call which took care of this but you said let all callers just
> > > handle it themselves.  So all callers submit a task and if they want to wait
> > > for the response they have to do so themselves.
> > 
> > Ah, true I remember that. The nice thing about a doing your own
> > wait_for_completion() like this is that you can make it
> > wait_for_completion_interruptible() to give up on the DOE if it gets
> > stalled. However, if you have a work item per-task and you're willing to
> > do an uninterruptible sleep, then flush_work(&task->work) is identical.
> 
> So when you mentioned a work item per task I really jumped on that idea.  But I
> realize now that it is a bit more complicated than that.
> 
> Currently a work item is actually one step of the state machine.  The state
> machine queues the next step of work as a new work item.
> 
> I'm going to have to change the state machine quite a bit.  I still agree with
> the one work item per task but it is going to take a bit of work to get the
> state machine to operate within that single task.
> 
> I don't like what might result if I layer a work queue on top of using the
> system work queue for the individual steps of the state machine.  So stay
> tuned.

In the end only one workqueue should exist either a task queue (my first
preference) or a device-state queue (if the task queue turns out not to
fit), but neither of those use cases should be glomming onto the
unbounded system_wq. Keep it simple with a dedicated ordered queue.