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;
From:   "Yu, Fenghua" <fenghua.yu@intel.com>
To:     "Chatre, Reinette" <reinette.chatre@intel.com>,
        "Jiang, Dave" <dave.jiang@intel.com>,
        "vkoul@kernel.org" <vkoul@kernel.org>,
        "dmaengine@vger.kernel.org" <dmaengine@vger.kernel.org>
CC:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH 3/3] dmaengine: idxd: Do not call DMX TX callbacks during
 workqueue disable
Thread-Topic: [PATCH 3/3] dmaengine: idxd: Do not call DMX TX callbacks during
 workqueue disable
Thread-Index: AQHZBnt1PdyLJW2z60eGK4vKGNplmq5bFvTg
Date:   Fri, 2 Dec 2022 21:12:51 +0000
Message-ID: <IA1PR11MB6097E887BBCE14CE2D47CDDB9B179@IA1PR11MB6097.namprd11.prod.outlook.com>
References: <cover.1670005163.git.reinette.chatre@intel.com>
 <93b5d144bfc16e0c0f640d5f7cfaeda6bf08753f.1670005163.git.reinette.chatre@intel.com>
In-Reply-To: <93b5d144bfc16e0c0f640d5f7cfaeda6bf08753f.1670005163.git.reinette.chatre@intel.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?1M0qPwpxu+DtI1Ddt3PoXo98l5RYAG7MXQNOOPL3l9xfP+bOrIUTQbDGEip7?=
 =?us-ascii?Q?3Lf4TAsCNkyLG2PcOjNKPaF58m6Q3WoiUf/v5/MKlY3aV8uxtf/krG6L3hYS?=
 =?us-ascii?Q?o5dMWCxihB3UVWC4KZ0FpXN0Za8hxKLZaxRlllzgUJthol524ypYajN21xTL?=
 =?us-ascii?Q?rVP3pvDvatdjD33Ta1LtaADa9Jb0i7rld7f9+Srs7j+H/QZHvkaIdJhhZMrN?=
 =?us-ascii?Q?5y2ykrxHdSt0nE3L3fCv/+mi9pw61YFa/HS6V5QK/+N3sYkysDJqqiuGzVO3?=
 =?us-ascii?Q?FpRqMUmCk5gvNfZtIV3MowjhyLkeFL6zodmfwL8vVT2IY5e83TvQ9+lyxGJG?=
 =?us-ascii?Q?QzYkUZk1q3g5yNSKU4reoKz6YRwo86nyW94v8cNrbjZUiywHdveF3Pe/JVMt?=
 =?us-ascii?Q?6pKAUM8gBL6/P890nduGJnq9fAIcEVMcQdwAq2kjKhDzClKlGQztibAxYYh1?=
 =?us-ascii?Q?LHgQcueN2Rw9t/l/iU8Hq/bQIySrdZcFAf+I43TAs+lSO00jXuot1nfFsruK?=
 =?us-ascii?Q?ggSNxF0uzt/WDDQy5KR9ELfsPNcd6snYmvYe4AGaIZX5vg11Cd8CDnMdJ1Pd?=
 =?us-ascii?Q?8fuDqeskE17039WkEKybdaaqKnMFXqKkFVKsOzPq9+4E2O6MU0h7Dbgt/PEY?=
 =?us-ascii?Q?AYrHmbyH3f6R7thFSCAuGoppu78HIOQ2RsbA5p8Dpi5CYeVHM5IlWQeCDzKu?=
 =?us-ascii?Q?OAzpmP6DNlZiy2AoBkdY7lV8ipwUm1soN+FRslDJzY9rYiM1TuttqylAB+wZ?=
 =?us-ascii?Q?yufbs/k/utag6/N0oX4DKxO3eHthC02sWXBkTjoMsIvoe+dd90yeKqkBoi6i?=
 =?us-ascii?Q?Dh2AckSnyAiVPiqM7j8xuoVRU9HTXw/FdKpM+qT1sBeFsRYcXnk312Z/NLZg?=
 =?us-ascii?Q?uXSYqBpqmVAbgyvi5e6iVzvYng3GbE8ye7HpowIkUUrGtE1YF9dADByhV2y+?=
 =?us-ascii?Q?UFK0NP1L/BFZNva4qATGiIviewGaEb7JMX1gvqW/7RoscRN1hpZDkFxIzePv?=
 =?us-ascii?Q?g8XjZ+yAvcNQu4HgUMKU+kdrgT1eJfuVhqQGQU5BMTKF9bSJR6YY9SXUGkj4?=
 =?us-ascii?Q?enq597gJJa9229JITsIOnplCbC8gm/QjLKAtCmxpcOZQQAr7P1SIh7h4O7uB?=
 =?us-ascii?Q?7P06btzyBSYhvqffQA4TyaSl8G2q6rocMO5qJPr31rXPL7zmovZHpthak6au?=
 =?us-ascii?Q?FurLDfcsSa2cpCP/tnR6QQx7ASy90uawIOSi6+puMOti7hXFoZylOiQNupDX?=
 =?us-ascii?Q?/BBb5t8zcAe6tBnxgHpt5t/nFPPrqZnqQFd5ESIQ2Uvd2KbNCA2ZyO2UhzU3?=
 =?us-ascii?Q?rcxVJSOjVXWtiXDzMjI3XoOWITdRwDDYPRttLu8Jez7Jp5yHvHkKgUKX8unr?=
 =?us-ascii?Q?KF9FJqWl9qDA1HjsLmjKjLa4Nsc18t0OeQOCCJx8r6e+a2XE3E8iOzvWjFal?=
 =?us-ascii?Q?qrlmbmcnY8sSC2nS23MqpeaWsZCeuFdMgfnR2A9huHnG47nh7co09BT1yKb/?=
 =?us-ascii?Q?1ySOwnbXUUhfTMQVmm0TQzPyZCw72tfwCIb6L0f0eur3rzxRhmrgu+kPHHza?=
 =?us-ascii?Q?v7aBLWKH2+p2Q3ec+uO4UwoJGb7d661ZY7F8cZex?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: IA1PR11MB6097.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 6d22272b-6cb4-48a1-2d4f-08dad4a9f894
X-MS-Exchange-CrossTenant-originalarrivaltime: 02 Dec 2022 21:12:51.1212
 (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: oy5ya3PAjNEZqFRPZQXnA+g/Jlps4PpNyqqooTQzLHGowDCKKQtyOUmvimUehp4IhW0K6Nd8Exvx2BFj8q/khA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR11MB7367
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-7.1 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 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

Hi, Reinette,

> On driver unload any pending descriptors are flushed and pending DMA
> descriptors are explicitly completed:
> idxd_dmaengine_drv_remove() ->
> 	drv_disable_wq() ->
> 		idxd_wq_free_irq() ->
> 			idxd_flush_pending_descs() ->
> 				idxd_dma_complete_txd()
>=20
> With this done during driver unload any remaining descriptor is likely st=
uck and
> can be dropped. Even so, the descriptor may still have a callback set tha=
t could
> no longer be accessible. An example of such a problem is when the dmatest=
 fails
> and the dmatest module is unloaded. The failure of dmatest leaves descrip=
tors
> with dma_async_tx_descriptor::callback pointing to code that no longer ex=
ist.
> This causes a page fault as below at the time the IDXD driver is unloaded=
 when it
> attempts to run the callback:
>  BUG: unable to handle page fault for address: ffffffffc0665190
>  #PF: supervisor instruction fetch in kernel mode
>  #PF: error_code(0x0010) - not-present page
>=20
> Fix this by clearing the callback pointers on the transmit descriptors on=
ly when
> workqueue is disabled.
>=20
> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
> ---
>=20
> History of refactoring made the Fixes: hard to identify by me.
>=20
>  drivers/dma/idxd/device.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>=20
> diff --git a/drivers/dma/idxd/device.c b/drivers/dma/idxd/device.c index
> b4d7bb923a40..2ac71a34fa34 100644
> --- a/drivers/dma/idxd/device.c
> +++ b/drivers/dma/idxd/device.c
> @@ -1156,6 +1156,7 @@ int idxd_device_load_config(struct idxd_device *idx=
d)
>=20
>  static void idxd_flush_pending_descs(struct idxd_irq_entry *ie)  {
> +	struct dma_async_tx_descriptor *tx;

Nitpicking. It's better to move this line to below:

>  	struct idxd_desc *desc, *itr;
>  	struct llist_node *head;
>  	LIST_HEAD(flist);
> @@ -1175,6 +1176,15 @@ static void idxd_flush_pending_descs(struct
> idxd_irq_entry *ie)
>  	list_for_each_entry_safe(desc, itr, &flist, list) {

here?
+	struct dma_async_tx_descriptor *tx;

>  		list_del(&desc->list);
>  		ctype =3D desc->completion->status ?
> IDXD_COMPLETE_NORMAL : IDXD_COMPLETE_ABORT;
> +		/*
> +		 * wq is being disabled. Any remaining descriptors are
> +		 * likely to be stuck and can be dropped. callback could
> +		 * point to code that is no longer accessible, for example
> +		 * if dmatest module has been unloaded.
> +		 */
> +		tx =3D &desc->txd;
> +		tx->callback =3D NULL;
> +		tx->callback_result =3D NULL;
>  		idxd_dma_complete_txd(desc, ctype, true);
>  	}
>  }
> --
> 2.34.1

Reviewed-by: Fenghua Yu <fenghua.yu@intel.com>

Thanks.

-Fenghua