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, 31 May 2023 14:56:33 -0700
From:   Dan Williams <dan.j.williams@intel.com>
To:     "Verma, Vishal L" <vishal.l.verma@intel.com>,
        "Williams, Dan J" <dan.j.williams@intel.com>,
        "Schofield, Alison" <alison.schofield@intel.com>,
        "Jiang, Dave" <dave.jiang@intel.com>,
        "bwidawsk@kernel.org" <bwidawsk@kernel.org>,
        "Weiny, Ira" <ira.weiny@intel.com>
CC:     "linux-cxl@vger.kernel.org" <linux-cxl@vger.kernel.org>,
        "dave@stgolabs.net" <dave@stgolabs.net>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "Weight, Russell H" <russell.h.weight@intel.com>
Subject: Re: [PATCH 3/4] cxl: add a firmware update mechanism using the sysfs
 firmware loader
Message-ID: <6477c29121ae3_168e29438@dwillia2-xfh.jf.intel.com.notmuch>
References: <20230421-vv-fw_update-v1-0-22468747d72f@intel.com>
 <20230421-vv-fw_update-v1-3-22468747d72f@intel.com>
 <646c313f20907_33fb329412@dwillia2-xfh.jf.intel.com.notmuch>
 <a7443a348b9c2b51cf141ad1131c9befbb09724e.camel@intel.com>
Content-Type: text/plain; charset="iso-8859-1"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <a7443a348b9c2b51cf141ad1131c9befbb09724e.camel@intel.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?iso-8859-1?Q?cpzmBmotlP6d/+LfgchVc1/idiJjKNlobbMw76Jv/swKntg4ZWbqFuCqWd?=
 =?iso-8859-1?Q?PRvWxPwOnPjbrcx142WGiY4RFjUb9K1PUpu0+4vH3CvHJf+6qdCo5M2Ksn?=
 =?iso-8859-1?Q?aEvLiRtkyHOXTFs9NGmtU1pCNa8GWdFbGjgTNNioOV58MTcPsD3PtyM2X8?=
 =?iso-8859-1?Q?5cEKGOlEDdUqogDEnjMZ84e5E2QecelDxZq0bPY8Sy4mVbNpzu/etlG9aq?=
 =?iso-8859-1?Q?PQZ5gYqU6upRLPTN3u+WWzAKO2HyhHcqdxGE7/8ZfIZkA7GQsoqxtxNP2A?=
 =?iso-8859-1?Q?YkfeuWlRdzrI/je1NgRHmQVTSD63ZTpAkigJKAYvOhYceTlDq1MyTVrsh+?=
 =?iso-8859-1?Q?UlJradi9GzfONgf8k2GbVH7qRuqmKiJsrostOsQcOpGLQeHxQmtj7cBRsF?=
 =?iso-8859-1?Q?JQHIMZouIGKrJ9gexhJ60yFwMz09QNg6dBm/08Ww/Tnmcb0FgtunQFIFvQ?=
 =?iso-8859-1?Q?kZaPecZ7om8Nu4ie53CX+El37fgHNVbgzK7nmhHLi4SuDBLa29zFhZ3MQ8?=
 =?iso-8859-1?Q?DtnchRHTJ5ZPvENN+xfD9D0IdfaZPpwsHX78Gc9PO/s4OybibHvNtC7miJ?=
 =?iso-8859-1?Q?TEWNvHhpcif1SyewIZpiTgxIuFXV7zDXCVheXR8SQ2NSIm23x90gxBXdKD?=
 =?iso-8859-1?Q?zQoNyLcowjq6qzSeeB6gp7c3U5eT3QxONAPjlRJUeJRiRuumOYoRxUvYVt?=
 =?iso-8859-1?Q?vcH8WQyApw1EvGauNOgk/MgknjgYx9XlV7Kq+BKkIn5vKcB1gSaF2ax2bq?=
 =?iso-8859-1?Q?NVdB179Q1XrlaDyC8yhSKrLttWLI3j+jNrGkHAW6wbKGM3VjGT+bkE8siu?=
 =?iso-8859-1?Q?9EpF6tF2grJ/EeQuTFTu2uSD/MPtPVJ2xCLtbku89EUZgDaJQTb6g+4KYr?=
 =?iso-8859-1?Q?wKoC8rrGv7Uq171NubR1OFjBYXyYSBNgP64TcJuaYP9atVZutwRL60f5qe?=
 =?iso-8859-1?Q?DVCUlr2u4COxWU0rliR/NYHrZ8t36rTtseVHajrNmVnWjkM7WpqFAmCoG8?=
 =?iso-8859-1?Q?mFEx8lC6Na9JxkdJ2V26soUID/HXT5/Qf+sVBZ0QfxMI2wP+UYgsSmBQuU?=
 =?iso-8859-1?Q?9Sb+vJkDZgYTLI8bC9C66MnlU42s4gEtBZr0sstBkOxGGDfrkFgHB0d5Ap?=
 =?iso-8859-1?Q?J/aor8A8FsHMSpGE4KikCtLCSGrXq02XctbMU6YZjGmzv2wmPexqW80PRp?=
 =?iso-8859-1?Q?JvM6Fqoukgj1X1YHnITDBOMlc9h7imTzDXSskqa9geELKX/DT1D5Vd0Bnc?=
 =?iso-8859-1?Q?rXt+wRzg1TtSyOEQP4vO8nNJ8mLcdooVw/LoNo/4qBmVCjNRNiHA52naNH?=
 =?iso-8859-1?Q?6ZqHsqGNbjTGVRfUfMKRyj+PcdBYvVgQamtGF9rMOPbCsFoBUV6Upzt/qb?=
 =?iso-8859-1?Q?AcGlTcwXKuTkkXYlOVNAVg4zqmCi7jPZVpxpJHOE0DTwTsy8wusIcU8JSu?=
 =?iso-8859-1?Q?mJjcJFiN/ZozDXkAroPqUk4NUYjDwZ46APmJlzcBkBDVItG+V+GkmJlbVm?=
 =?iso-8859-1?Q?bUeKbN0Fejr9OSFbWpb9YIMdAaneHgNznU+KpBP9mSJ6zRCL13qhM7J6w2?=
 =?iso-8859-1?Q?sSwvRu1oZ2zdSCHmHvZh89GA/5NLmx3fIUTSSdGt5dJBxq2dFzUYHh5nn5?=
 =?iso-8859-1?Q?G3bAV8T56iDj0UySb0nEuiWfwcL+kbV2mTkzO7sRsoHSbd0zcfsv4BoQ?=
 =?iso-8859-1?Q?=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 719ec02d-2e42-4884-3fac-08db6221e735
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 31 May 2023 21:56:35.7424
 (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: qcAdjFSPU5fFFVpTWYo5NJpp2ZesfCV5JJfidDHscuE1qbbmJmxvq3CIlCLQrQNrzD9Nz7IIv7698L6q7Xgb3kuVU9eOLFF2xNJWR6ORiYc=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL1PR11MB5287
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.6 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,URIBL_BLOCKED
        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

Verma, Vishal L wrote:
> On Mon, 2023-05-22 at 20:21 -0700, Dan Williams wrote:
> > Vishal Verma wrote:
> 
> <snip>
> Everything else not addressed here sounds good and I've made those
> changes.
> 
> > > 
> > > +???????remaining = size - cur_size;
> > > +???????size_in = cur_size + sizeof(*transfer);
> > > +
> > > +???????mutex_lock(&cxlds->fw.fw_mutex);
> > 
> > What is this lock protecting? I.e. will the fw_loader really try to send
> > multiple overlapping firmware update attempts?
> 
> The lock is just to provide predictable points at which a cancel
> request may be intercepted. The loader won't try overlapping firmware
> transfer requests, but the ->cancel request comes from user space, and
> could happen while there is a transfer in progress. With the lock, the
> cancel will only be 'processed' after the current chunk's transfer is
> done. 

So right now cancel is only considered at certain points within either
the ->write() or ->poll_complete() callbacks. The firmware upload core
is guaranteeing that ->prepare(), ->write() and ->poll_complete() never
overlap for a given session, and that if any of those return an error
the upload session is terminated.

While the lock does flush in flight ->write() and ->prepare() it does
nothing to enforce when the cancellation is processed. It will still be
the case that the next invocation of ->write() or ->poll_complete() will
consider the cancel state before doing the next step.

I am failing to see what the lock is protecting. The other usage is for
checking that ->prepare() has completed before ->write() is invoked, but
again that is enforced by the firmware uploader workqueue.

I think the lock and the clear_to_send bit can be eliminated. Clear to
send is implied by ->prepare() succeeding. Convert cancel to an atomic
flag where cxl_fw_cancel() does:

set_bit(CXL_FW_CANCEL, &cxlds->fw.state);

...and cxl_fw_write() and cxl_fw_poll_complete() can just do:

if (test_and_clear_bit(CXL_FW_CANCEL, &cxlds->fw.state))
	do_cancel();