From mboxrd@z Thu Jan 1 00:00:00 1970 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 6273C274FD0 for ; Mon, 20 Jul 2026 02:09:45 +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=1784513386; cv=none; b=WVPEY2m1HjUHLL1kFsQ7+FNeAnf5cao19/M/2jFi+VinEKP9Zom3x6IjluJHQEPcC3RG1OSNxM8evNZ2DO4n9rI630kz6xlA7AzJ+2KSnUe/ckQSPJ3SviqmTnildlW/LiYCOh4wp4e/nNzk/0Lg7WsSD4NE1pzegAK3m7F3WWM= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1784513386; c=relaxed/simple; bh=LOq550oZdYMXzH9HST84dp+4UmnsdWmFQkhaMMLPCS4=; h=Date:From:To:Cc:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=mo9yAhbjxfEuuy4pMXLoo1+JO9qthc8CDAFu1U90w6VDN1tt8nK8GONtWpAb8QI6Cs3ChUGkBeAPgju8Et+cOIDOzfN0I/VuNZGtUPjRjKSl/i+Sy5vLpDlYQcHEgbKbnY/ZTRqhiOeRW4G2R+dPdu5jfVPYki+GPVMomhlLmZM= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=oss.qualcomm.com; spf=pass smtp.mailfrom=oss.qualcomm.com; dkim=pass (2048-bit key) header.d=qualcomm.com header.i=@qualcomm.com header.b=d7zQVrz8; dkim=pass (2048-bit key) header.d=oss.qualcomm.com header.i=@oss.qualcomm.com header.b=azgtgalU; arc=none smtp.client-ip=205.220.168.131 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=oss.qualcomm.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=oss.qualcomm.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=qualcomm.com header.i=@qualcomm.com header.b="d7zQVrz8"; dkim=pass (2048-bit key) header.d=oss.qualcomm.com header.i=@oss.qualcomm.com header.b="azgtgalU" Received: from pps.filterd (m0279867.ppops.net [127.0.0.1]) by mx0a-0031df01.pphosted.com (8.18.1.11/8.18.1.11) with ESMTP id 66JNtVE81913615 for ; Mon, 20 Jul 2026 02:09:44 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=qualcomm.com; h= cc:content-transfer-encoding:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to; s=qcppdkim1; bh= 007+nLYN72ZEaozMMOfz5QZ6KNBrKf3sjeslstsi8cw=; b=d7zQVrz8uw29Ce3B BDptq0dwrDbq1hD6RSEeNdasDtKHXgWEof8TlVOtabw1VM9pw/29jz68Bv5L+E36 zBFU985QcJS+ZSHkiJHMNkSFqskZloENIAE/4yoN0Ft0YuzyZxLvn+Z5LbUQ6uYw e1Wvatyh+EFek96+CGPHS/p1gcv+JbKJhjBg93o0ebuem2NrOR/8hnb9hmFGOA9c /UUJGsH+2kPLnwg8JETZsDe9LqE0qkdBQuuC+MEL6mDKyvnh8eQQ/FNWvwWFFmIJ UgNt9qluv86zH6mlRsqu1vKpAILNi15yOOoAkYXyolcyGZAPI5q7cNjmDvWkptbs Fp+yaQ== Received: from mail-pl1-f198.google.com (mail-pl1-f198.google.com [209.85.214.198]) by mx0a-0031df01.pphosted.com (PPS) with ESMTPS id 4fg2aauwy5-1 (version=TLSv1.3 cipher=TLS_AES_128_GCM_SHA256 bits=128 verify=NOT) for ; Mon, 20 Jul 2026 02:09:44 +0000 (GMT) Received: by mail-pl1-f198.google.com with SMTP id d9443c01a7336-2ce7dfd33ffso120388665ad.0 for ; Sun, 19 Jul 2026 19:09:44 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oss.qualcomm.com; s=google; t=1784513384; x=1785118184; darn=vger.kernel.org; h=content-transfer-encoding:content-type:mime-version:references :in-reply-to:message-id:subject:cc:to:from:date:from:to:cc:subject :date:message-id:reply-to:content-type; bh=007+nLYN72ZEaozMMOfz5QZ6KNBrKf3sjeslstsi8cw=; b=azgtgalUaOXBlr2VkcGknLNZED31adYOYKF7PImmrfrl4XlzpSMaZn5kuzMLiJd0aT 2txYDSLqY/ZiTvf7oLFQjIIgLGc37R7B7vjm8ImswXPlFO45EmGShJW5CuK/GhmyuoKI b7JI7PUCqzsz1Sw2Mxtw+3hyLcBAdTJVRXWjp9u4jDAr1CELhT3Uq1odesyHh4uyukgF imlulX5gxvBduw9qK3QDZox5sA55MjK6odmON/3kVIPXpzPnn1QyvjnmRYBfPCl3cGr3 puysV4Zh9o7duBxccMvawsA/jYoZdrDIJMRVLDaVs5AuQhjpRI2r3TDgykqvbblZwX0Z wCpg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1784513384; x=1785118184; h=content-transfer-encoding:content-type:mime-version:references :in-reply-to:message-id:subject:cc:to:from:date:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to :content-type; bh=007+nLYN72ZEaozMMOfz5QZ6KNBrKf3sjeslstsi8cw=; b=l8dHtY7KdNyYzghLVVMUdr7iUeY3+Sp43PmFl4cURB8dwnzrcjf69jFHWfcnMFzHPH qLJVa6wj7ddZz/Q6pk9aeU5QW/q1DYVCVmQLoHrpkq9vNnf4lj9QyX+7T7ZRJ2MLN2Uc +gNEOjL13lS1HzhatWFLj3qD/q3pNxgGle43aSI1qyGrXoleSAzI7rgHzMmE+XeDbrtX oAReCuMf0QVM2/P7VU+YSCUrmk36URmasQ6VekmHgvz/v+ZYJ7iy920xthya4KiKwCJx XwYi3pUB6DdEXty7hky1h8qzljdk77LhNyjuSaPz7sCqGTudlr3qYoK2cmgDUg6Amz0Y bKiQ== X-Forwarded-Encrypted: i=1; AHgh+Rq9r2gRUZVhAJueriXMkugKmYOJKYN/P+XAfOHkS0hVlzCrH7nZmy5TRl/6rbSzRBQSFDOfQbsFqgGSwv4=@vger.kernel.org X-Gm-Message-State: AOJu0YxR7H0v5QCpG0/zhF3CAq8kshxP8LXO0tZgjV67UvQAuaQHLRyM BrMxaD105YT2/B8/lMKUaJeFb7Lj97Nesklwon5wuxPGx4XEMf8JfyKDBAvHpXpdL5jO6Peo2V/ +1OrxP2ro0Y6g3m0Yzkyv59BM9IhQ+Esx/Nao7LHwQ6Ua5oPxNt8ZAwXH/0sy7VUUc8o= X-Gm-Gg: AfdE7cmse7Yb8kCCed0tgCqQbFdAf7885czHqH/hbAxxothj5ZstiaFujlrlwmahd2G eSphSbBXrkCiG7jqJ3IIgW2tZhZ/58UqpFv2jjzLTjr563QYkUus8TSLCt196SFFmHLCZ3bwYu4 li/nR+O8ZBy2X1huHnkTWT8sCzhUFdw5RfyKS7pSQXFIFnT4y4b3wqSS8OMf4u36eUpVm7I46R2 g9QT31WUtmYf3qVuazDP0zRw2sBBQP0hsGTOt3GykzZMadNqP7OTE94IO3d1l2PfgncXBkJA03F WR078A9Ime/sG8UX+wY1O34bPb6w8tImYPJYAkKBfwPY68qM86ZNCtQl57w5iCuPCRWjVSQARNX klqPGxooJKBZz5y9q X-Received: by 2002:a17:902:da8d:b0:2ca:10c6:f69b with SMTP id d9443c01a7336-2cf3482135fmr135999455ad.5.1784513383745; Sun, 19 Jul 2026 19:09:43 -0700 (PDT) X-Received: by 2002:a17:902:da8d:b0:2ca:10c6:f69b with SMTP id d9443c01a7336-2cf3482135fmr135999045ad.5.1784513383225; Sun, 19 Jul 2026 19:09:43 -0700 (PDT) Received: from jic23-huawei ([50.35.46.84]) by smtp.gmail.com with ESMTPSA id d9443c01a7336-2cf346d9ba9sm48395085ad.48.2026.07.19.19.09.41 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 19 Jul 2026 19:09:42 -0700 (PDT) Date: Mon, 20 Jul 2026 03:09:38 +0100 From: Jonathan Cameron To: Biren Pandya Cc: linusw@kernel.org, dlechner@baylibre.com, nuno.sa@analog.com, andy@kernel.org, linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v3] iio: gyro: mpu3050: Fix runtime PM leak and refactor trigger state Message-ID: <20260720030938.697b5a2b@jic23-huawei> In-Reply-To: <20260714131426.4257-2-birenpandya@gmail.com> References: <20260615214504.38979-1-birenpandya@gmail.com> <20260714131426.4257-2-birenpandya@gmail.com> X-Mailer: Claws Mail 4.4.0 (GTK 3.24.52; x86_64-pc-linux-gnu) Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-Proofpoint-GUID: -4OVv4vp6GS3RMkeq-RKPnF2oF4lG8hS X-Proofpoint-Spam-Info: AW1haW4tMjYwNzIwMDAyMCBTYWx0ZWRfX0wsxGFbVlE8B b6+bt3N6UKVBF8jfF/Gcdv3RiquhCnjis6cFwu4A9+tb67I8SKQJCb14DNFx5Es5C2F5ruaes2z plgY6W7V+lTgzTWYnRk87YWxG8Efs0s= X-Authority-Analysis: v=2.4 cv=b9aCJNGx c=1 sm=1 tr=0 ts=6a5d8368 cx=c_pps a=MTSHoo12Qbhz2p7MsH1ifg==:117 a=qC1CW/w66vtJz1P9yTJxNA==:17 a=kj9zAlcOel0A:10 a=RAioF0-LDSMA:10 a=s4-Qcg_JpJYA:10 a=VkNPw1HP01LnGYTKEx00:22 a=u7WPNUs3qKkmUXheDGA7:22 a=eoimf2acIAo5FJnRuUoq:22 a=VwQbUJbxAAAA:8 a=pGLkceISAAAA:8 a=mUsWOzX0D04toIqhuc4A:9 a=CjuIK1q_8ugA:10 a=GvdueXVYPmCkWapjIL-Q:22 X-Proofpoint-ORIG-GUID: -4OVv4vp6GS3RMkeq-RKPnF2oF4lG8hS X-Proofpoint-Spam-Details-Enc: AW1haW4tMjYwNzIwMDAyMCBTYWx0ZWRfX1g+WXaNjK3ll jpHCI3RKn58rvbEr0HD5m4nRVD8l6L+tfm6VOQ0rf6l6LX2XKP/tH6Z40dkXjX6VfLok1XXyu2T 76SzTd+fflqTyYUT6Nms5pADyoeeFb14tlWFHyxWmd+A5wjb8fJip9iuyS/petjflbIUlJFNjac nvC3eJiHa+ClvO8EtLjUG6rkPyM/I/gooa2J9QgWeTPFfkAir9zvREApduUqvIf2Vqd2zVc0tzE spGCN7DQ2rGtGiSZSaQHbFPeJGQURwsHVzkFOmfjiKVZv+vkhnGKVZWj4h5GvqvyHijBnjTytr8 PrbRkFFdLwmN5PukLHct0ayhORLDzaUx8owU+0r0XrMHIqGuVXN8FlQsAadRZP4IbuJFejeZQ6m FYRoIgC5NAdXgQ7A2UmtcYm9jT2uUHRRcXfQs+aEFHOQkBSxi7XowZTIP3/HXUoWvF/7iiTk84k wFkSHnZUd3LkQxf3Awg== X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.293,Aquarius:18.0.1143,Hydra:6.1.134,FMLib:17.12.100.49 definitions=2026-07-19_08,2026-07-17_01,2025-10-01_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 impostorscore=0 suspectscore=0 phishscore=0 priorityscore=1501 lowpriorityscore=0 clxscore=1015 adultscore=0 malwarescore=0 bulkscore=0 spamscore=0 classifier=typeunknown authscore=0 authtc= authcc= route=outbound adjust=0 reason=mlx scancount=1 engine=8.22.0-2606150000 definitions=main-2607200020 On Tue, 14 Jul 2026 18:44:27 +0530 Biren Pandya wrote: > mpu3050_drdy_trigger_set_state() calls pm_runtime_get_sync() when the > trigger is enabled, but several error paths in the enable branch return > directly without dropping the usage counter again. pm_runtime_get_sync() > increments the usage counter, so every failed enable leaks a runtime PM > reference and the device can no longer autosuspend. The driver state flag > hw_irq_trigger is also left set after a failed enable. > > To fix the error unwind clearly and avoid an asymmetric goto block inside a > monolithic function, this patch breaks the trigger state handler into two > distinct helpers: mpu3050_drdy_trigger_enable() and > mpu3050_drdy_trigger_disable(). The enable helper correctly implements the > error unwind path to drop the PM reference and clear the flag. > > Additionally, pm_runtime_get_sync() is replaced with > pm_runtime_resume_and_get() for robust error checking. > > Fixes: 3904b28efb2c ("iio: gyro: Add driver for the MPU-3050 gyroscope") > Signed-off-by: Biren Pandya Hmm. I just replied to an ancient version... Reason being you keep sending new series in response to old ones. Do not do that as it means simple date sorting fails! Anyhow, some comments inline for this approach. > --- > > Changes in v3: > - Fixed kernel-doc warning for mpu3050_drdy_trigger_set_state(). > - Fixed the Fixes tag title to exactly match the target commit. > - Link to v2: https://lore.kernel.org/all/20260615214504.38979-1-birenpandya@gmail.com/ > drivers/iio/gyro/mpu3050-core.c | 166 ++++++++++++++++++-------------- > 1 file changed, 92 insertions(+), 74 deletions(-) > > diff --git a/drivers/iio/gyro/mpu3050-core.c b/drivers/iio/gyro/mpu3050-core.c > index d84e04e4b4314..9de126c3b4350 100644 > --- a/drivers/iio/gyro/mpu3050-core.c > +++ b/drivers/iio/gyro/mpu3050-core.c > @@ -945,102 +945,120 @@ static irqreturn_t mpu3050_irq_thread(int irq, void *p) > return IRQ_HANDLED; > } > > -/** > - * mpu3050_drdy_trigger_set_state() - set data ready interrupt state > - * @trig: trigger instance > - * @enable: true if trigger should be enabled, false to disable > - */ > -static int mpu3050_drdy_trigger_set_state(struct iio_trigger *trig, > - bool enable) > +static int mpu3050_drdy_trigger_disable(struct iio_trigger *trig) > { > struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig); > struct mpu3050 *mpu3050 = iio_priv(indio_dev); > unsigned int val; > int ret; > > - /* Disabling trigger: disable interrupt and return */ > - if (!enable) { > - /* Disable all interrupts */ > - ret = regmap_write(mpu3050->map, > - MPU3050_INT_CFG, > - 0); > - if (ret) > - dev_err(mpu3050->dev, "error disabling IRQ\n"); > + /* Disable all interrupts */ > + ret = regmap_write(mpu3050->map, MPU3050_INT_CFG, 0); > + if (ret) > + dev_err(mpu3050->dev, "error disabling IRQ\n"); If this fails we loose the error. Just exit on each error we are effectively in an unknown and probably dead state anyway if only some of these work. There are some error paths where the only right answer is to reset the driver by an unbind rebind. > > - /* Clear IRQ flag */ > - ret = regmap_read(mpu3050->map, MPU3050_INT_STATUS, &val); > - if (ret) > - dev_err(mpu3050->dev, "error clearing IRQ status\n"); > + /* Clear IRQ flag */ > + ret = regmap_read(mpu3050->map, MPU3050_INT_STATUS, &val); > + if (ret) > + dev_err(mpu3050->dev, "error clearing IRQ status\n"); > > - /* Disable all things in the FIFO and reset it */ > - ret = regmap_write(mpu3050->map, MPU3050_FIFO_EN, 0); > - if (ret) > - dev_err(mpu3050->dev, "error disabling FIFO\n"); > + /* Disable all things in the FIFO and reset it */ > + ret = regmap_write(mpu3050->map, MPU3050_FIFO_EN, 0); > + if (ret) > + dev_err(mpu3050->dev, "error disabling FIFO\n"); > > - ret = regmap_write(mpu3050->map, MPU3050_USR_CTRL, > - MPU3050_USR_CTRL_FIFO_RST); > - if (ret) > - dev_err(mpu3050->dev, "error resetting FIFO\n"); > + ret = regmap_write(mpu3050->map, MPU3050_USR_CTRL, > + MPU3050_USR_CTRL_FIFO_RST); > + if (ret) > + dev_err(mpu3050->dev, "error resetting FIFO\n"); > > - pm_runtime_put_autosuspend(mpu3050->dev); > - mpu3050->hw_irq_trigger = false; > + pm_runtime_put_autosuspend(mpu3050->dev); > + mpu3050->hw_irq_trigger = false; > > - return 0; > - } else { > - /* Else we're enabling the trigger from this point */ > - pm_runtime_get_sync(mpu3050->dev); > - mpu3050->hw_irq_trigger = true; > + return 0; If it failed in any way we should not be returning 0. Sure there isn't a lot we can do to recover but this hides the problem from upper layers of the stack. > +} > > - /* Disable all things in the FIFO */ > - ret = regmap_write(mpu3050->map, MPU3050_FIFO_EN, 0); > - if (ret) > - return ret; > +static int mpu3050_drdy_trigger_enable(struct iio_trigger *trig) > +{ > + struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig); > + struct mpu3050 *mpu3050 = iio_priv(indio_dev); > + unsigned int val; > + int ret; > > - /* Reset and enable the FIFO */ > - ret = regmap_set_bits(mpu3050->map, MPU3050_USR_CTRL, > - MPU3050_USR_CTRL_FIFO_EN | > - MPU3050_USR_CTRL_FIFO_RST); > - if (ret) > - return ret; > + ret = pm_runtime_resume_and_get(mpu3050->dev); > + if (ret) > + return ret; > > - mpu3050->pending_fifo_footer = false; > + mpu3050->hw_irq_trigger = true; Seems like an oddly early place to do this but I guess this is what the original code did for some reason. If you can figure that out add a comment on why this isn't left until we know we successfully turned the trigger on. > > - /* Turn on the FIFO for temp+X+Y+Z */ > - ret = regmap_write(mpu3050->map, MPU3050_FIFO_EN, > - MPU3050_FIFO_EN_TEMP_OUT | > - MPU3050_FIFO_EN_GYRO_XOUT | > - MPU3050_FIFO_EN_GYRO_YOUT | > - MPU3050_FIFO_EN_GYRO_ZOUT | > - MPU3050_FIFO_EN_FOOTER); > - if (ret) > - return ret; > + /* Disable all things in the FIFO */ > + ret = regmap_write(mpu3050->map, MPU3050_FIFO_EN, 0); > + if (ret) > + goto err_put_autosuspend; > > - /* Configure the sample engine */ > - ret = mpu3050_start_sampling(mpu3050); > - if (ret) > - return ret; > + /* Reset and enable the FIFO */ > + ret = regmap_set_bits(mpu3050->map, MPU3050_USR_CTRL, > + MPU3050_USR_CTRL_FIFO_EN | > + MPU3050_USR_CTRL_FIFO_RST); > + if (ret) > + goto err_put_autosuspend; > > - /* Clear IRQ flag */ > - ret = regmap_read(mpu3050->map, MPU3050_INT_STATUS, &val); > - if (ret) > - dev_err(mpu3050->dev, "error clearing IRQ status\n"); > + mpu3050->pending_fifo_footer = false; > > - /* Give us interrupts whenever there is new data ready */ > - val = MPU3050_INT_RAW_RDY_EN; > + /* Turn on the FIFO for temp+X+Y+Z */ > + ret = regmap_write(mpu3050->map, MPU3050_FIFO_EN, > + MPU3050_FIFO_EN_TEMP_OUT | > + MPU3050_FIFO_EN_GYRO_XOUT | > + MPU3050_FIFO_EN_GYRO_YOUT | > + MPU3050_FIFO_EN_GYRO_ZOUT | > + MPU3050_FIFO_EN_FOOTER); > + if (ret) > + goto err_put_autosuspend; > > - if (mpu3050->irq_actl) > - val |= MPU3050_INT_ACTL; > - if (mpu3050->irq_latch) > - val |= MPU3050_INT_LATCH_EN; > - if (mpu3050->irq_opendrain) > - val |= MPU3050_INT_OPEN; > + /* Configure the sample engine */ > + ret = mpu3050_start_sampling(mpu3050); > + if (ret) > + goto err_put_autosuspend; > > - ret = regmap_write(mpu3050->map, MPU3050_INT_CFG, val); > - if (ret) > - return ret; > - } > + /* Clear IRQ flag */ > + ret = regmap_read(mpu3050->map, MPU3050_INT_STATUS, &val); > + if (ret) > + dev_err(mpu3050->dev, "error clearing IRQ status\n"); > + > + /* Give us interrupts whenever there is new data ready */ > + val = MPU3050_INT_RAW_RDY_EN; > + > + if (mpu3050->irq_actl) > + val |= MPU3050_INT_ACTL; > + if (mpu3050->irq_latch) > + val |= MPU3050_INT_LATCH_EN; > + if (mpu3050->irq_opendrain) > + val |= MPU3050_INT_OPEN; > + > + ret = regmap_write(mpu3050->map, MPU3050_INT_CFG, val); > + if (ret) > + goto err_put_autosuspend; > > return 0; > + > +err_put_autosuspend: > + pm_runtime_put_autosuspend(mpu3050->dev); > + mpu3050->hw_irq_trigger = false; As above. Just move the setting of this until just above the return 0 and no need to reset it. There might be a reason though so do check the driver carefully for how this is used. > + return ret; > +} > + > +/** > + * mpu3050_drdy_trigger_set_state() - set data ready interrupt state > + * @trig: trigger instance > + * @enable: true if trigger should be enabled, false to disable > + */ > +static int mpu3050_drdy_trigger_set_state(struct iio_trigger *trig, > + bool enable) > +{ > + if (enable) > + return mpu3050_drdy_trigger_enable(trig); > + else > + return mpu3050_drdy_trigger_disable(trig); > } > > static const struct iio_trigger_ops mpu3050_trigger_ops = {