From patchwork Mon Aug 19 20:07:45 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Bjorn Andersson X-Patchwork-Id: 820583 Received: from mx0a-0031df01.pphosted.com (mx0a-0031df01.pphosted.com [205.220.168.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 391DF1DD39B; Mon, 19 Aug 2024 20:08:28 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=205.220.168.131 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1724098110; cv=none; b=TuNxCHlmDW+b3/SpJ7lVECE4lPq3A7l/Qn5Jqxk633K9o8sx8vtXMVnBoyjefrwLUKZeL4a/II2atTnrD9t+pAp4xvjhDWGJTF1dhX1oTPXB8PnoQBQMKlwV5NPgw+VOrKSlgwjR98OTDx9BeQ0ejgxK+neZrhQEo2nsqlEZBx4= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1724098110; c=relaxed/simple; bh=mBC/h/l9RjtodWk1nsZhY+oGspAZvoc28EsOZgvBRhw=; h=From:Date:Subject:MIME-Version:Content-Type:Message-ID:References: In-Reply-To:To:CC; b=N3rvk3qOQSe5K4LXiXuE4C5l40VZnDSNwEBQvNMM+yUZobrIfycRzcir1rOYo/TAGTIIgfB3tf57Qt++8rMFLASngH2XuOJPBVCyBuz6l2d9myDNwOp8rawD6jZI+CpVNKBb7dvHnMuj1MluaN3sT3DLSaX5GJqkVBT638lPSdw= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=quicinc.com; spf=pass smtp.mailfrom=quicinc.com; dkim=pass (2048-bit key) header.d=quicinc.com header.i=@quicinc.com header.b=FdNDopih; arc=none smtp.client-ip=205.220.168.131 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=quicinc.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=quicinc.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=quicinc.com header.i=@quicinc.com header.b="FdNDopih" Received: from pps.filterd (m0279862.ppops.net [127.0.0.1]) by mx0a-0031df01.pphosted.com (8.18.1.2/8.18.1.2) with ESMTP id 47JBgvXf031294; Mon, 19 Aug 2024 20:08:23 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=quicinc.com; h= cc:content-transfer-encoding:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to; s=qcppdkim1; bh= NMlujU8Sht5g1QAb62FU0NkEnGuE+hAIpL7SLQUfPSk=; b=FdNDopihQxs4n76U V6uk6YFHPhzhX4yazx/vvigeuT79Oqh1PWcDj1rAdQji/zraGVzHG6KIakTGu0Wb 9ZQ7fcTcFnW+NlWJ5IcUXu3+bsjUKyFkYhvvDG7vAWktulFWX+YsDy+ajHv1vtH5 Au1CHYlD8csaNRQX4CYg2u6NNmp3Bhq4vBfFunlcNg9deNzBeQ/ByFJHpi4FaX9M Ige48UKkHsB2d9vUwNqThOEJqHcmFjJnZATkbs8AN87yIbtnMiTOSryjDVzVLc2u nVfOtGtnhrqh7zTn/GxIHtWm5PdvhnUsEBmq2j75JgFeWgr9wHniRq3gqcjzdw4x BIDEfw== Received: from nalasppmta01.qualcomm.com (Global_NAT1.qualcomm.com [129.46.96.20]) by mx0a-0031df01.pphosted.com (PPS) with ESMTPS id 412mmen9bx-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 19 Aug 2024 20:08:23 +0000 (GMT) Received: from nalasex01c.na.qualcomm.com (nalasex01c.na.qualcomm.com [10.47.97.35]) by NALASPPMTA01.qualcomm.com (8.18.1.2/8.18.1.2) with ESMTPS id 47JK8MaG030447 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 19 Aug 2024 20:08:22 GMT Received: from hu-bjorande-lv.qualcomm.com (10.49.16.6) by nalasex01c.na.qualcomm.com (10.47.97.35) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.1544.9; Mon, 19 Aug 2024 13:08:22 -0700 From: Bjorn Andersson Date: Mon, 19 Aug 2024 13:07:45 -0700 Subject: [PATCH v2 1/3] soc: qcom: pmic_glink: Fix race during initialization Precedence: bulk X-Mailing-List: linux-usb@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Message-ID: <20240819-pmic-glink-v6-11-races-v2-1-88fe3ab1f0e2@quicinc.com> References: <20240819-pmic-glink-v6-11-races-v2-0-88fe3ab1f0e2@quicinc.com> In-Reply-To: <20240819-pmic-glink-v6-11-races-v2-0-88fe3ab1f0e2@quicinc.com> To: Sebastian Reichel , Bjorn Andersson , Konrad Dybcio , "Heikki Krogerus" , Greg Kroah-Hartman , Neil Armstrong CC: Johan Hovold , Chris Lew , Dmitry Baryshkov , Stephen Boyd , Amit Pundir , , , , , Bjorn Andersson , Johan Hovold , X-Mailer: b4 0.14.1 X-Developer-Signature: v=1; a=ed25519-sha256; t=1724098101; l=8590; i=quic_bjorande@quicinc.com; s=20230915; h=from:subject:message-id; bh=mBC/h/l9RjtodWk1nsZhY+oGspAZvoc28EsOZgvBRhw=; b=cuzi1r0yFLRbL5FON2ynWbUpy8FwUrpJ2s+ybKmyE2GciX+WXxCRnoLH370qIkP9rMLdyoatt Rh/k7RV57OdCDwo0LxK5C9AAobR93B6c0EmOxXdbJ+gh4p8zORBiaZh X-Developer-Key: i=quic_bjorande@quicinc.com; a=ed25519; pk=VkhObtljigy9k0ZUIE1Mvr0Y+E1dgBEH9WoLQnUtbIM= X-ClientProxiedBy: nalasex01a.na.qualcomm.com (10.47.209.196) To nalasex01c.na.qualcomm.com (10.47.97.35) X-QCInternal: smtphost X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=5800 signatures=585085 X-Proofpoint-GUID: zODlutEKdnAwCmuumbzjQq545e0C0dR3 X-Proofpoint-ORIG-GUID: zODlutEKdnAwCmuumbzjQq545e0C0dR3 X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.293,Aquarius:18.0.1039,Hydra:6.0.680,FMLib:17.12.28.16 definitions=2024-08-19_16,2024-08-19_03,2024-05-17_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 phishscore=0 clxscore=1015 mlxlogscore=999 priorityscore=1501 bulkscore=0 impostorscore=0 adultscore=0 mlxscore=0 malwarescore=0 lowpriorityscore=0 spamscore=0 suspectscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.19.0-2407110000 definitions=main-2408190137 As pointed out by Stephen Boyd it is possible that during initialization of the pmic_glink child drivers, the protection-domain notifiers fires, and the associated work is scheduled, before the client registration returns and as a result the local "client" pointer has been initialized. The outcome of this is a NULL pointer dereference as the "client" pointer is blindly dereferenced. Timeline provided by Stephen: CPU0 CPU1 ---- ---- ucsi->client = NULL; devm_pmic_glink_register_client() client->pdr_notify(client->priv, pg->client_state) pmic_glink_ucsi_pdr_notify() schedule_work(&ucsi->register_work) pmic_glink_ucsi_register() ucsi_register() pmic_glink_ucsi_read_version() pmic_glink_ucsi_read() pmic_glink_ucsi_read() pmic_glink_send(ucsi->client) ucsi->client = client // Too late! This code is identical across the altmode, battery manager and usci child drivers. Resolve this by splitting the allocation of the "client" object and the registration thereof into two operations. This only happens if the protection domain registry is populated at the time of registration, which by the introduction of commit '1ebcde047c54 ("soc: qcom: add pd-mapper implementation")' became much more likely. Reported-by: Amit Pundir Closes: https://lore.kernel.org/all/CAMi1Hd2_a7TjA7J9ShrAbNOd_CoZ3D87twmO5t+nZxC9sX18tA@mail.gmail.com/ Reported-by: Johan Hovold Closes: https://lore.kernel.org/all/ZqiyLvP0gkBnuekL@hovoldconsulting.com/ Reported-by: Stephen Boyd Closes: https://lore.kernel.org/all/CAE-0n52JgfCBWiFQyQWPji8cq_rCsviBpW-m72YitgNfdaEhQg@mail.gmail.com/ Fixes: 58ef4ece1e41 ("soc: qcom: pmic_glink: Introduce base PMIC GLINK driver") Cc: stable@vger.kernel.org Reviewed-by: Heikki Krogerus Reviewed-by: Neil Armstrong Tested-by: Amit Pundir Signed-off-by: Bjorn Andersson Reviewed-by: Johan Hovold --- drivers/power/supply/qcom_battmgr.c | 16 ++++++++++------ drivers/soc/qcom/pmic_glink.c | 28 ++++++++++++++++++---------- drivers/soc/qcom/pmic_glink_altmode.c | 17 +++++++++++------ drivers/usb/typec/ucsi/ucsi_glink.c | 16 ++++++++++------ include/linux/soc/qcom/pmic_glink.h | 11 ++++++----- 5 files changed, 55 insertions(+), 33 deletions(-) diff --git a/drivers/power/supply/qcom_battmgr.c b/drivers/power/supply/qcom_battmgr.c index 49bef4a5ac3f..df90a470c51a 100644 --- a/drivers/power/supply/qcom_battmgr.c +++ b/drivers/power/supply/qcom_battmgr.c @@ -1387,12 +1387,16 @@ static int qcom_battmgr_probe(struct auxiliary_device *adev, "failed to register wireless charing power supply\n"); } - battmgr->client = devm_pmic_glink_register_client(dev, - PMIC_GLINK_OWNER_BATTMGR, - qcom_battmgr_callback, - qcom_battmgr_pdr_notify, - battmgr); - return PTR_ERR_OR_ZERO(battmgr->client); + battmgr->client = devm_pmic_glink_new_client(dev, PMIC_GLINK_OWNER_BATTMGR, + qcom_battmgr_callback, + qcom_battmgr_pdr_notify, + battmgr); + if (IS_ERR(battmgr->client)) + return PTR_ERR(battmgr->client); + + pmic_glink_register_client(battmgr->client); + + return 0; } static const struct auxiliary_device_id qcom_battmgr_id_table[] = { diff --git a/drivers/soc/qcom/pmic_glink.c b/drivers/soc/qcom/pmic_glink.c index 9ebc0ba35947..58ec91767d79 100644 --- a/drivers/soc/qcom/pmic_glink.c +++ b/drivers/soc/qcom/pmic_glink.c @@ -66,15 +66,14 @@ static void _devm_pmic_glink_release_client(struct device *dev, void *res) spin_unlock_irqrestore(&pg->client_lock, flags); } -struct pmic_glink_client *devm_pmic_glink_register_client(struct device *dev, - unsigned int id, - void (*cb)(const void *, size_t, void *), - void (*pdr)(void *, int), - void *priv) +struct pmic_glink_client *devm_pmic_glink_new_client(struct device *dev, + unsigned int id, + void (*cb)(const void *, size_t, void *), + void (*pdr)(void *, int), + void *priv) { struct pmic_glink_client *client; struct pmic_glink *pg = dev_get_drvdata(dev->parent); - unsigned long flags; client = devres_alloc(_devm_pmic_glink_release_client, sizeof(*client), GFP_KERNEL); if (!client) @@ -85,6 +84,18 @@ struct pmic_glink_client *devm_pmic_glink_register_client(struct device *dev, client->cb = cb; client->pdr_notify = pdr; client->priv = priv; + INIT_LIST_HEAD(&client->node); + + devres_add(dev, client); + + return client; +} +EXPORT_SYMBOL_GPL(devm_pmic_glink_new_client); + +void pmic_glink_register_client(struct pmic_glink_client *client) +{ + struct pmic_glink *pg = client->pg; + unsigned long flags; mutex_lock(&pg->state_lock); spin_lock_irqsave(&pg->client_lock, flags); @@ -95,11 +106,8 @@ struct pmic_glink_client *devm_pmic_glink_register_client(struct device *dev, spin_unlock_irqrestore(&pg->client_lock, flags); mutex_unlock(&pg->state_lock); - devres_add(dev, client); - - return client; } -EXPORT_SYMBOL_GPL(devm_pmic_glink_register_client); +EXPORT_SYMBOL_GPL(pmic_glink_register_client); int pmic_glink_send(struct pmic_glink_client *client, void *data, size_t len) { diff --git a/drivers/soc/qcom/pmic_glink_altmode.c b/drivers/soc/qcom/pmic_glink_altmode.c index 1e0808b3cb93..e4f5059256e5 100644 --- a/drivers/soc/qcom/pmic_glink_altmode.c +++ b/drivers/soc/qcom/pmic_glink_altmode.c @@ -520,12 +520,17 @@ static int pmic_glink_altmode_probe(struct auxiliary_device *adev, return ret; } - altmode->client = devm_pmic_glink_register_client(dev, - altmode->owner_id, - pmic_glink_altmode_callback, - pmic_glink_altmode_pdr_notify, - altmode); - return PTR_ERR_OR_ZERO(altmode->client); + altmode->client = devm_pmic_glink_new_client(dev, + altmode->owner_id, + pmic_glink_altmode_callback, + pmic_glink_altmode_pdr_notify, + altmode); + if (IS_ERR(altmode->client)) + return PTR_ERR(altmode->client); + + pmic_glink_register_client(altmode->client); + + return 0; } static const struct auxiliary_device_id pmic_glink_altmode_id_table[] = { diff --git a/drivers/usb/typec/ucsi/ucsi_glink.c b/drivers/usb/typec/ucsi/ucsi_glink.c index 16c328497e0b..ac53a81c2a81 100644 --- a/drivers/usb/typec/ucsi/ucsi_glink.c +++ b/drivers/usb/typec/ucsi/ucsi_glink.c @@ -367,12 +367,16 @@ static int pmic_glink_ucsi_probe(struct auxiliary_device *adev, ucsi->port_orientation[port] = desc; } - ucsi->client = devm_pmic_glink_register_client(dev, - PMIC_GLINK_OWNER_USBC, - pmic_glink_ucsi_callback, - pmic_glink_ucsi_pdr_notify, - ucsi); - return PTR_ERR_OR_ZERO(ucsi->client); + ucsi->client = devm_pmic_glink_new_client(dev, PMIC_GLINK_OWNER_USBC, + pmic_glink_ucsi_callback, + pmic_glink_ucsi_pdr_notify, + ucsi); + if (IS_ERR(ucsi->client)) + return PTR_ERR(ucsi->client); + + pmic_glink_register_client(ucsi->client); + + return 0; } static void pmic_glink_ucsi_remove(struct auxiliary_device *adev) diff --git a/include/linux/soc/qcom/pmic_glink.h b/include/linux/soc/qcom/pmic_glink.h index fd124aa18c81..aedde76d7e13 100644 --- a/include/linux/soc/qcom/pmic_glink.h +++ b/include/linux/soc/qcom/pmic_glink.h @@ -23,10 +23,11 @@ struct pmic_glink_hdr { int pmic_glink_send(struct pmic_glink_client *client, void *data, size_t len); -struct pmic_glink_client *devm_pmic_glink_register_client(struct device *dev, - unsigned int id, - void (*cb)(const void *, size_t, void *), - void (*pdr)(void *, int), - void *priv); +struct pmic_glink_client *devm_pmic_glink_new_client(struct device *dev, + unsigned int id, + void (*cb)(const void *, size_t, void *), + void (*pdr)(void *, int), + void *priv); +void pmic_glink_register_client(struct pmic_glink_client *client); #endif From patchwork Mon Aug 19 20:07:46 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Bjorn Andersson X-Patchwork-Id: 821289 Received: from mx0a-0031df01.pphosted.com (mx0a-0031df01.pphosted.com [205.220.168.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id B15C81DD39C; Mon, 19 Aug 2024 20:08:29 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=205.220.168.131 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1724098111; cv=none; b=Ni/80XCkR0gJVKVPDehKqz1OxbJDgE5XkQiDjKQh53bdbwpv0yLZo8o9dmfIBc3Lmun/SJ6sa5yvAIvoAXdg9jVKu3uv7bVXc1zWkUIovb9dgAW3WAPaic0IwGrEhSXu7xnZMNuRFEpwhaVRl5jE9SowGmOaYVqUMnS/6mCoNig= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1724098111; c=relaxed/simple; bh=VMywmaV2PC3+xgr4QRnO1wOmoE97PYvg4j7eQMJZDJg=; h=From:Date:Subject:MIME-Version:Content-Type:Message-ID:References: In-Reply-To:To:CC; b=PARhBP8kj94qF3+84brnDSNaY8iJ7AJ3oSbt68HTYM2JFmQqq1D+yF+idLY0lFXI0RZkk8ZmsnALfylLB823mupqSaMCj8Sxg/PDv9i1rtwJbjGGDohazanmhW8ow9+bI8mkRBS/l7ZzZvDIz3sxb4GEWql7PTWr9FQB4MVQMKg= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=quicinc.com; spf=pass smtp.mailfrom=quicinc.com; dkim=pass (2048-bit key) header.d=quicinc.com header.i=@quicinc.com header.b=krpEkZ1b; arc=none smtp.client-ip=205.220.168.131 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=quicinc.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=quicinc.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=quicinc.com header.i=@quicinc.com header.b="krpEkZ1b" Received: from pps.filterd (m0279863.ppops.net [127.0.0.1]) by mx0a-0031df01.pphosted.com (8.18.1.2/8.18.1.2) with ESMTP id 47JBA2Ze022742; Mon, 19 Aug 2024 20:08:24 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=quicinc.com; h= cc:content-transfer-encoding:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to; s=qcppdkim1; bh= gZAnlzRN7R8Xgp/8ZFX6752e8W3mZV1F/mkGlAWZ5gc=; b=krpEkZ1bts/NCHMp aHeJWcvpoCN2Ftcg+n01dZdC49kL4thjX+x0AzX5IHtpHR7NcLS0SXtQOx6Shjkr NQ618YlAAXfXYFSMrDQqnr+9F+g+ueZGeSYZ5sqMQR1BHpXDmI+ymq+GomwJ4RW0 cuE34idqBBmPJxNjSSI0xXdHqr2uKa4rbhjRSBFoct0euSDPrz11SrjK4eXoB5rW /g3fcmo4EcNdSF7SxB9A1gP8XPT1KUfiwLxisv5GgMWDFL1R1de6k5MtKFJI/aFt 6FdUn/bnYTrUIXcC127q8GKdpa5EDovGfirdNuV1lUD4Y8iGWz5d6wpvDpmChj/d +hhcBQ== Received: from nalasppmta05.qualcomm.com (Global_NAT1.qualcomm.com [129.46.96.20]) by mx0a-0031df01.pphosted.com (PPS) with ESMTPS id 412m32ncy6-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 19 Aug 2024 20:08:23 +0000 (GMT) Received: from nalasex01c.na.qualcomm.com (nalasex01c.na.qualcomm.com [10.47.97.35]) by NALASPPMTA05.qualcomm.com (8.18.1.2/8.18.1.2) with ESMTPS id 47JK8MJA019555 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 19 Aug 2024 20:08:22 GMT Received: from hu-bjorande-lv.qualcomm.com (10.49.16.6) by nalasex01c.na.qualcomm.com (10.47.97.35) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.1544.9; Mon, 19 Aug 2024 13:08:22 -0700 From: Bjorn Andersson Date: Mon, 19 Aug 2024 13:07:46 -0700 Subject: [PATCH v2 2/3] usb: typec: ucsi: Move unregister out of atomic section Precedence: bulk X-Mailing-List: linux-usb@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Message-ID: <20240819-pmic-glink-v6-11-races-v2-2-88fe3ab1f0e2@quicinc.com> References: <20240819-pmic-glink-v6-11-races-v2-0-88fe3ab1f0e2@quicinc.com> In-Reply-To: <20240819-pmic-glink-v6-11-races-v2-0-88fe3ab1f0e2@quicinc.com> To: Sebastian Reichel , Bjorn Andersson , Konrad Dybcio , "Heikki Krogerus" , Greg Kroah-Hartman , Neil Armstrong CC: Johan Hovold , Chris Lew , Dmitry Baryshkov , Stephen Boyd , Amit Pundir , , , , , Bjorn Andersson , X-Mailer: b4 0.14.1 X-Developer-Signature: v=1; a=ed25519-sha256; t=1724098101; l=4450; i=quic_bjorande@quicinc.com; s=20230915; h=from:subject:message-id; bh=VMywmaV2PC3+xgr4QRnO1wOmoE97PYvg4j7eQMJZDJg=; b=YPEj0ic2lN+vc/rh4jE5Sn8CEocbodrM1U1FhY+i/Ye1mVIyi57uW2Uglyc5QfdDDiclMt+9I o7n5qxTXaY2CruFadRpr4r0rv/nFnlExm7OI6tGLZPdmtkfln2iIEFb X-Developer-Key: i=quic_bjorande@quicinc.com; a=ed25519; pk=VkhObtljigy9k0ZUIE1Mvr0Y+E1dgBEH9WoLQnUtbIM= X-ClientProxiedBy: nalasex01a.na.qualcomm.com (10.47.209.196) To nalasex01c.na.qualcomm.com (10.47.97.35) X-QCInternal: smtphost X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=5800 signatures=585085 X-Proofpoint-GUID: NtI19OLslqDwQ7WWPHLcSLRzxfbQ6WSy X-Proofpoint-ORIG-GUID: NtI19OLslqDwQ7WWPHLcSLRzxfbQ6WSy X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.293,Aquarius:18.0.1039,Hydra:6.0.680,FMLib:17.12.28.16 definitions=2024-08-19_16,2024-08-19_03,2024-05-17_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 malwarescore=0 mlxscore=0 suspectscore=0 priorityscore=1501 phishscore=0 bulkscore=0 lowpriorityscore=0 impostorscore=0 clxscore=1015 mlxlogscore=999 spamscore=0 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.19.0-2407110000 definitions=main-2408190137 Commit '635ce0db8956 ("soc: qcom: pmic_glink: don't traverse clients list without a lock")' moved the pmic_glink client list under a spinlock, as it is accessed by the rpmsg/glink callback, which in turn is invoked from IRQ context. This means that ucsi_unregister() is now called from IRQ context, which isn't feasible as it's expecting a sleepable context. An effort is under way to get GLINK to invoke its callbacks in a sleepable context, but until then lets schedule the unregistration. A side effect of this is that ucsi_unregister() can now happen after the remote processor, and thereby the communication link with it, is gone. pmic_glink_send() is amended with a check to avoid the resulting NULL pointer dereference. This does however result in the user being informed about this error by the following entry in the kernel log: ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: failed to send UCSI write request: -5 Fixes: 635ce0db8956 ("soc: qcom: pmic_glink: don't traverse clients list without a lock") Cc: stable@vger.kernel.org Reviewed-by: Heikki Krogerus Reviewed-by: Neil Armstrong Reviewed-by: Dmitry Baryshkov Tested-by: Amit Pundir Signed-off-by: Bjorn Andersson --- drivers/soc/qcom/pmic_glink.c | 10 +++++++++- drivers/usb/typec/ucsi/ucsi_glink.c | 27 ++++++++++++++++++++++----- 2 files changed, 31 insertions(+), 6 deletions(-) diff --git a/drivers/soc/qcom/pmic_glink.c b/drivers/soc/qcom/pmic_glink.c index 58ec91767d79..e4747f1d3da5 100644 --- a/drivers/soc/qcom/pmic_glink.c +++ b/drivers/soc/qcom/pmic_glink.c @@ -112,8 +112,16 @@ EXPORT_SYMBOL_GPL(pmic_glink_register_client); int pmic_glink_send(struct pmic_glink_client *client, void *data, size_t len) { struct pmic_glink *pg = client->pg; + int ret; - return rpmsg_send(pg->ept, data, len); + mutex_lock(&pg->state_lock); + if (!pg->ept) + ret = -ECONNRESET; + else + ret = rpmsg_send(pg->ept, data, len); + mutex_unlock(&pg->state_lock); + + return ret; } EXPORT_SYMBOL_GPL(pmic_glink_send); diff --git a/drivers/usb/typec/ucsi/ucsi_glink.c b/drivers/usb/typec/ucsi/ucsi_glink.c index ac53a81c2a81..bb6244f21e0a 100644 --- a/drivers/usb/typec/ucsi/ucsi_glink.c +++ b/drivers/usb/typec/ucsi/ucsi_glink.c @@ -68,6 +68,9 @@ struct pmic_glink_ucsi { struct work_struct notify_work; struct work_struct register_work; + spinlock_t state_lock; + bool ucsi_registered; + bool pd_running; u8 read_buf[UCSI_BUF_SIZE]; }; @@ -244,8 +247,20 @@ static void pmic_glink_ucsi_notify(struct work_struct *work) static void pmic_glink_ucsi_register(struct work_struct *work) { struct pmic_glink_ucsi *ucsi = container_of(work, struct pmic_glink_ucsi, register_work); + unsigned long flags; + bool pd_running; - ucsi_register(ucsi->ucsi); + spin_lock_irqsave(&ucsi->state_lock, flags); + pd_running = ucsi->pd_running; + spin_unlock_irqrestore(&ucsi->state_lock, flags); + + if (!ucsi->ucsi_registered && pd_running) { + ucsi_register(ucsi->ucsi); + ucsi->ucsi_registered = true; + } else if (ucsi->ucsi_registered && !pd_running) { + ucsi_unregister(ucsi->ucsi); + ucsi->ucsi_registered = false; + } } static void pmic_glink_ucsi_callback(const void *data, size_t len, void *priv) @@ -269,11 +284,12 @@ static void pmic_glink_ucsi_callback(const void *data, size_t len, void *priv) static void pmic_glink_ucsi_pdr_notify(void *priv, int state) { struct pmic_glink_ucsi *ucsi = priv; + unsigned long flags; - if (state == SERVREG_SERVICE_STATE_UP) - schedule_work(&ucsi->register_work); - else if (state == SERVREG_SERVICE_STATE_DOWN) - ucsi_unregister(ucsi->ucsi); + spin_lock_irqsave(&ucsi->state_lock, flags); + ucsi->pd_running = state == SERVREG_SERVICE_STATE_UP; + spin_unlock_irqrestore(&ucsi->state_lock, flags); + schedule_work(&ucsi->register_work); } static void pmic_glink_ucsi_destroy(void *data) @@ -320,6 +336,7 @@ static int pmic_glink_ucsi_probe(struct auxiliary_device *adev, INIT_WORK(&ucsi->register_work, pmic_glink_ucsi_register); init_completion(&ucsi->read_ack); init_completion(&ucsi->write_ack); + spin_lock_init(&ucsi->state_lock); mutex_init(&ucsi->lock); ucsi->ucsi = ucsi_create(dev, &pmic_glink_ucsi_ops); From patchwork Mon Aug 19 20:07:47 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Bjorn Andersson X-Patchwork-Id: 820582 Received: from mx0a-0031df01.pphosted.com (mx0a-0031df01.pphosted.com [205.220.168.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 7678E1DD39D; Mon, 19 Aug 2024 20:08:30 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=205.220.168.131 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1724098111; cv=none; b=NfvH/RLRppb+hFOpJFPmFZu58V4fcmCx584nL0l5aT9yrXbBD7M+N3HTXPFtA536t8FlUfdJXLRFd3UOynQ3JqOIwmgj22FX86tOV4hi6P+kq5STuH5qgVOyhCPAQgU+mFGjrqKImnVL9WRrEJ7AWsVSPnDkGV1WyUW57eSfAII= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1724098111; c=relaxed/simple; bh=AzBRnzE5wF8BxaGA/G/dnXeF8ct7PClnoIzeb8NYXKg=; h=From:Date:Subject:MIME-Version:Content-Type:Message-ID:References: In-Reply-To:To:CC; b=O/GeuotzDh8jw1glI+kHnKFAshRl263YaKh4yJtFNeqMb3u0GsQ4pnAGpc4jt/jyImLbzvuGguyjiwzapapP4qyhM8nD73yFa+1dBnJeA972CcGGJTBxrewKA9biqTsW7hDPCsmk4XCcSGt+gWzdZOhUPyPzU8QLCAk/Fa1Cu0s= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=quicinc.com; spf=pass smtp.mailfrom=quicinc.com; dkim=pass (2048-bit key) header.d=quicinc.com header.i=@quicinc.com header.b=HgAvFGfG; arc=none smtp.client-ip=205.220.168.131 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=quicinc.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=quicinc.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=quicinc.com header.i=@quicinc.com header.b="HgAvFGfG" Received: from pps.filterd (m0279865.ppops.net [127.0.0.1]) by mx0a-0031df01.pphosted.com (8.18.1.2/8.18.1.2) with ESMTP id 47JAon8L025131; Mon, 19 Aug 2024 20:08:24 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=quicinc.com; h= cc:content-transfer-encoding:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to; s=qcppdkim1; bh= psX4yoMM8/JkY1r0sAHQnk6Qzp2O5lOgQ4lDw0sU6iE=; b=HgAvFGfGutRGw8S4 iWbJMm7+PfO2wyxlkSi0B1YRC7Buzr0EE8sDNIYQRS28DFD8WOwfDxlEt0E6WSKd +ywJFOyPb0Zd9tciRUWYFxnjRh3ZrNVBs6FsX6MbsKV1XfBlYLNQFNeSxS4qAJZn XyIuaD77/uh87GNNRJE5wHpZvnPUI9ovb8+zXu74x6byLa9d+iWzCS+Tp83SouEi Y6bE5VXbnWTBNqDG2pFCSpml3WL6EoIGt1hUR5gyWuGkvK4jzZmsRaj5hseJDfI/ 4RvvjcBEYcxNsrkaPZJJmYEuz/AceMr49jjzG1RN3CoXanKwZ/A8JockRnT57aZR FCSEGg== Received: from nalasppmta03.qualcomm.com (Global_NAT1.qualcomm.com [129.46.96.20]) by mx0a-0031df01.pphosted.com (PPS) with ESMTPS id 412jtrwesg-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 19 Aug 2024 20:08:24 +0000 (GMT) Received: from nalasex01c.na.qualcomm.com (nalasex01c.na.qualcomm.com [10.47.97.35]) by NALASPPMTA03.qualcomm.com (8.18.1.2/8.18.1.2) with ESMTPS id 47JK8NAR029674 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 19 Aug 2024 20:08:23 GMT Received: from hu-bjorande-lv.qualcomm.com (10.49.16.6) by nalasex01c.na.qualcomm.com (10.47.97.35) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.1544.9; Mon, 19 Aug 2024 13:08:22 -0700 From: Bjorn Andersson Date: Mon, 19 Aug 2024 13:07:47 -0700 Subject: [PATCH v2 3/3] soc: qcom: pmic_glink: Actually communicate with remote goes down Precedence: bulk X-Mailing-List: linux-usb@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Message-ID: <20240819-pmic-glink-v6-11-races-v2-3-88fe3ab1f0e2@quicinc.com> References: <20240819-pmic-glink-v6-11-races-v2-0-88fe3ab1f0e2@quicinc.com> In-Reply-To: <20240819-pmic-glink-v6-11-races-v2-0-88fe3ab1f0e2@quicinc.com> To: Sebastian Reichel , Bjorn Andersson , Konrad Dybcio , "Heikki Krogerus" , Greg Kroah-Hartman , Neil Armstrong CC: Johan Hovold , Chris Lew , Dmitry Baryshkov , Stephen Boyd , Amit Pundir , , , , , Bjorn Andersson , X-Mailer: b4 0.14.1 X-Developer-Signature: v=1; a=ed25519-sha256; t=1724098101; l=1515; i=quic_bjorande@quicinc.com; s=20230915; h=from:subject:message-id; bh=AzBRnzE5wF8BxaGA/G/dnXeF8ct7PClnoIzeb8NYXKg=; b=T1Kps4TJ8SRx5AQtzKZIC9kRu7K+7MInTTRHTHjNHEMxuNwEvHIzPoXhCknRbcPN7j3MUjScU bPz7Ok+SzcOByClxpcdMv5AS5MtFVbdXN7JW0aEkC7+nvcLEiO8ep+z X-Developer-Key: i=quic_bjorande@quicinc.com; a=ed25519; pk=VkhObtljigy9k0ZUIE1Mvr0Y+E1dgBEH9WoLQnUtbIM= X-ClientProxiedBy: nalasex01a.na.qualcomm.com (10.47.209.196) To nalasex01c.na.qualcomm.com (10.47.97.35) X-QCInternal: smtphost X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=5800 signatures=585085 X-Proofpoint-ORIG-GUID: GNw6EbgvQmreIWRha_L_AjW6nYobysUn X-Proofpoint-GUID: GNw6EbgvQmreIWRha_L_AjW6nYobysUn X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.293,Aquarius:18.0.1039,Hydra:6.0.680,FMLib:17.12.28.16 definitions=2024-08-19_16,2024-08-19_03,2024-05-17_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 clxscore=1015 suspectscore=0 bulkscore=0 mlxlogscore=999 mlxscore=0 impostorscore=0 malwarescore=0 lowpriorityscore=0 adultscore=0 priorityscore=1501 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.19.0-2407110000 definitions=main-2408190137 When the pmic_glink state is UP and we either receive a protection- domain (PD) notification indicating that the PD is going down, or that the whole remoteproc is going down, it's expected that the pmic_glink client instances are notified that their function has gone DOWN. This is not what the code does, which results in the client state either not updating, or being wrong in many cases. So let's fix the conditions. Fixes: 58ef4ece1e41 ("soc: qcom: pmic_glink: Introduce base PMIC GLINK driver") Cc: stable@vger.kernel.org Reviewed-by: Heikki Krogerus Reviewed-by: Neil Armstrong Reviewed-by: Dmitry Baryshkov Tested-by: Amit Pundir Signed-off-by: Bjorn Andersson Reviewed-by: Johan Hovold --- drivers/soc/qcom/pmic_glink.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/soc/qcom/pmic_glink.c b/drivers/soc/qcom/pmic_glink.c index e4747f1d3da5..cb202a37e8ab 100644 --- a/drivers/soc/qcom/pmic_glink.c +++ b/drivers/soc/qcom/pmic_glink.c @@ -191,7 +191,7 @@ static void pmic_glink_state_notify_clients(struct pmic_glink *pg) if (pg->pdr_state == SERVREG_SERVICE_STATE_UP && pg->ept) new_state = SERVREG_SERVICE_STATE_UP; } else { - if (pg->pdr_state == SERVREG_SERVICE_STATE_UP && pg->ept) + if (pg->pdr_state == SERVREG_SERVICE_STATE_DOWN || !pg->ept) new_state = SERVREG_SERVICE_STATE_DOWN; }