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:   Wed, 22 Jun 2022 17:25:02 -0700
From:   Ira Weiny <ira.weiny@intel.com>
To:     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: <YrOy3vodu3FcRjUY@iweiny-desk3>
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>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <62b39e5eead1_32f38a294b6@dwillia2-xfh.notmuch>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?KskN203duuqARywGEAOeVrU+MZimzfGuUZG7m17+Tc3fjXG0cS4Zm8iCQtDY?=
 =?us-ascii?Q?RzBhhndGShwZfoFHZU86rJ+PZ1tka1EPbYDWdme0Gw5QOFMbru1SfnhymdBv?=
 =?us-ascii?Q?b2qzenZ0Duyfroz86ZDDFVwY+NLyArLEqzhCS8CQ0i0hLyUJqE8yfKai6tPk?=
 =?us-ascii?Q?IgUQdPG6/B7uWHwcu81hWO/sRusF8OwDolmGp4+Sac9KXEGKl9EaBHbWnSIH?=
 =?us-ascii?Q?qCkCa/3Ku5dXOx7HrF+BiP9enJaDY/Vnhl7hey3m4wJIuXAQjzLyzmKZHSfO?=
 =?us-ascii?Q?hFFDV4AUrDbjLQWC0OqlNm3YluUzXLsphiwZvRLTmGA6uOpZ31cLzZBCWbyP?=
 =?us-ascii?Q?fchsvR7Egjvu7bg1xXVgxJpqP5iWoBXtQazAck0E1U/D+RmeIyABM0aRq5X/?=
 =?us-ascii?Q?jC4E63xQ81Br8DKpcXJ0CTfei4pm9Lp8JRQxBmC6fY41aSCTrI6d9s7OkQWo?=
 =?us-ascii?Q?SC4TgRT5y8+WeBy21EIutTyYc9exShGznAgaELncKgq+e4KjGXCZZm3ECzwq?=
 =?us-ascii?Q?Wji7amRsglvOdEGIDM5QRCyKecMFVq8eA+Dzs7rASyuqk9LibQOljTIWdlsY?=
 =?us-ascii?Q?WbVAn+wrvZqg1aMdCSlvoGn+J85X4rsBZSVLHa7bxFubF3EZ88aUXFnYGUKE?=
 =?us-ascii?Q?Sk9ASdNDMsKXOpno12DnHfUBldW+H0mmZMF1GbFe28pQnxsIFqMkeiaNoNxd?=
 =?us-ascii?Q?A1KCqLaIa7pYQ5NMD1A7jRWyM8oDSLLsXqDaryD8G0Hbm2ecEPiu+QHmCWA+?=
 =?us-ascii?Q?BeZvftvx01Z/D0Lec+2md2JkhMmzAD8sfauTK6nhQTRQaBA9/L6oUdTJYdEu?=
 =?us-ascii?Q?lpIDsf3I3oW1i+bnXLOOBwQ13fe54Vi5Klou2CkbovruywU35q+vm8M7kSex?=
 =?us-ascii?Q?4fHu7p6fHtgF9hBtS5PAHZeG6qqOIY8s2jS23Fx8mprif6Zj1VHwHxEPWp6x?=
 =?us-ascii?Q?XS7Wx3ZgrQky7+Kj8Dri5Ds72s3xLv1L/taBXtN55s93CCA9cMpyiiHxuTSD?=
 =?us-ascii?Q?bZuxmNBTupBRNQNZBbqIy3xwN6azdB5Atz9c+pKE4HfDyHl66NFUynQXa3by?=
 =?us-ascii?Q?IiUSG/GCaHsebFlH0ynrERkcIu9jgbNi1Kk0UA/ufWMbgdmzC0K1M6KFIdWW?=
 =?us-ascii?Q?9NF8KVWPWDxR1jXpkn60nq91Ul9HcKvYeVQ6pVhDVw1TJ9X9Ye8S3SauoR3g?=
 =?us-ascii?Q?+GkEXrXIK0JrYB83oUdNLH+YceKixErbUDWReeYBRmgXoEUq2gfScCF2nXWS?=
 =?us-ascii?Q?FIeuZ4r7M1AvYvXVmr114RRHhrFLTKJVxzXllK76JCrXAlBtuZ2xOtqWQgfj?=
 =?us-ascii?Q?aRgFZE96l8Kr36rL4ecAYLk8Ssyf0VntLmURE1XnWqXkrtf/IeOQVzZjRyc+?=
 =?us-ascii?Q?DWNvhtc950HPKgtq2ThkBYucGLNq/W4Mhj+i6uUC8tQScqUmjKnjvuAeLzDR?=
 =?us-ascii?Q?xdfoOYvIktEQM3FQHwqmUevuYaqz/JfKCIeZ4SNhMCjKVjAloCKL375iJRob?=
 =?us-ascii?Q?OTXGO5vrPH0DU0tKGGB2dtNYDtJUl1Hd1UbEi3FbntVaTkVNA0gl7Oub8cCh?=
 =?us-ascii?Q?rv4xMuzuIIVfisub4RrgFZ9yCKbyWc8Pt11JkDNKEh8rGqW2yn0lKYBJ5beE?=
 =?us-ascii?Q?kZQdM8nKZDlGw2MfcuJH6Dqq8miyDqigRkGfOlUIdC2CuoOv9mxBOj+pMy8n?=
 =?us-ascii?Q?NMQTrNl1mklM1zu2Q5D2sY4tG0jDULd3pactrVHAK45f1w7ByndrIp6OKa3i?=
 =?us-ascii?Q?4dpsNvjYtNN3p3XnD6TSQqx7I3uPgBU=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 8053734d-790b-4e74-0df6-08da54aed3d6
X-MS-Exchange-CrossTenant-AuthSource: DM4PR11MB6311.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Jun 2022 00:25:08.2516
 (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: ev2AolJYYfs/FKJap0bfVnR41o4l0fXx4qfMxy6sf51mYh+c5zNENh72c3GvYJHC9CorHsQRndx1uLDnv+gWYA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR11MB5536
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-7.7 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_HI,
        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

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.

Ira