From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-alma10-1.taild15c8.ts.net [100.103.45.18]) (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 1AAA44DD6FE for ; Tue, 14 Jul 2026 16:58:43 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=100.103.45.18 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1784048324; cv=none; b=GTEcU7lhXgXPu6cdFOzaG4M7RjUojSeflqn7r6sAcRV9jSiV5WbO0FpYSknhorY/riVJHfPzZwBQ/PcHmaURv9WrszCqBn8XM+DRFp2Fde3YvFZTrx2iGfBxm+LyKpKgbv0pZzC9k6kOrZ+zfWkMNGQD3ez7ECXQ6KkbDYHp8oM= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1784048324; c=relaxed/simple; bh=PMWkGEDy7EexoemkWJBbWz6CQNhUnQc1sG3UKHXKn4g=; h=From:Subject:To:Cc:In-Reply-To:References:Content-Type:Date: Message-Id; b=s3xVqRFdvurWQ8ExdbQLQWlFkLt2UAYaj494KwcTZyPkHm4WpL7gtHEupynwCLqi/ORjkGBUrNHFCIU3e4bpjMCBRijpf8OD1PfKDgBXJIwc3DCUrsjV0J7rOPnBavD150WWlLRKQh7lABWn8DYesOrkQxw2aCFSyerfgKTeWJ4= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=cYK+tjTO; arc=none smtp.client-ip=100.103.45.18 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="cYK+tjTO" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 923281F00A3A; Tue, 14 Jul 2026 16:58:42 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1784048323; bh=0pGunXII3U4EDPZqzX1qtVA0XvnXle3oGjJIllYWcgk=; h=From:Subject:Reply-To:To:Cc:In-Reply-To:References:Date; b=cYK+tjTOl7X/jIthqyWOled9s91E07w2CSpPuAWuXL47LTEQQWCPkEu/FAtk2Dz5b Izg71/0dADO6TbazdhPj2F3yWseayXMkySZH3gicgj+5n+kcRE8Ec/NdAiECzFmNaM XMDv31iDhxXEGeL2lHhspkuDYLESnWY0COtEE0SJ4hKv7oqGtff1tbAdGVrcKVZ92u L/DrAMXsvbNWVd6G2X95B9wP3Mh+h5iZRVZlxOR3T8LeFYnjJUdM8z206EfPQiuS5+ gcU//V+YOXj+Q0lFgpiWkPIcPQ5GUPpxt300w3XoNNrbEijGMQcC37rhsGbwdo/E+f r/UoyLM65SNCA== From: sashiko-bot@kernel.org Subject: Re: [PATCH] can: c_can: keep controller in init mode until bittiming is configured Reply-To: sashiko-reviews@lists.linux.dev To: "Lucas Martins Alves" Cc: linux-can@vger.kernel.org, "Oleksij Rempel" , "Vincent Mailhol" , "Marc Kleine-Budde" , "Oliver Hartkopp" In-Reply-To: <20260714164839.771123-1-lucas.alves@lumal21.com.br> References: <20260714164839.771123-1-lucas.alves@lumal21.com.br> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Date: Tue, 14 Jul 2026 16:58:42 +0000 Message-Id: <20260714165842.923281F00A3A@smtp.kernel.org> Precedence: bulk X-Mailing-List: linux-can@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: Thank you for your contribution! Sashiko AI review found 1 potential issue(= s) to consider: Pre-existing issues: - [High] Configuring bit timing temporarily clears the CONTROL_TEST bit, wh= ich silently resets the TEST register and breaks loopback/listen-only modes. -- --- Patch [1]: [PATCH] can: c_can: keep controller in init mode until bitti= ming is configured --- commit b7bd06d474febb092c757ff6270ed7de9b22644e Author: Lucas Martins Alves can: c_can: keep controller in init mode until bittiming is configured c_can_chip_config() was programming C_CAN_CTRL_REG without CONTROL_INIT, which may allow the controller to become active before c_can_set_bittiming() finishes. This patch adds CONTROL_INIT to the writes in c_can_chip_config() so the controller stays halted until bit timing is fully programmed. > diff --git a/drivers/net/can/c_can/c_can_main.c b/drivers/net/can/c_can/c= _can_main.c > index 3702cac7fbf0f..32a3975b9d692 100644 > --- a/drivers/net/can/c_can/c_can_main.c > +++ b/drivers/net/can/c_can/c_can_main.c > @@ -597,20 +597,20 @@ static int c_can_chip_config(struct net_device *dev) [ ... ] > if ((priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY) && > (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK)) { > /* loopback + silent mode : useful for hot self-test */ > - priv->write_reg(priv, C_CAN_CTRL_REG, CONTROL_TEST); > + priv->write_reg(priv, C_CAN_CTRL_REG, CONTROL_TEST | CONTROL_INIT); > priv->write_reg(priv, C_CAN_TEST_REG, TEST_LBACK | TEST_SILENT); > } else if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK) { [Severity: High] This is a pre-existing issue, but does the loopback or listen-only test mode actually survive the initialization process? When we configure C_CAN_TEST_REG here in c_can_chip_config(), we set the te= st bits. However, shortly after this block, the function calls c_can_set_bittiming() to configure the timing: drivers/net/can/c_can/c_can_main.c:c_can_set_bittiming() { ... ctrl_save =3D priv->read_reg(priv, C_CAN_CTRL_REG); ctrl_save &=3D ~CONTROL_INIT; priv->write_reg(priv, C_CAN_CTRL_REG, CONTROL_CCE | CONTROL_INIT); ... priv->write_reg(priv, C_CAN_CTRL_REG, ctrl_save); } When writing CONTROL_CCE | CONTROL_INIT to the control register, the CONTROL_TEST bit is explicitly omitted. Doesn't clearing the CONTROL_TEST b= it silently reset the C_CAN_TEST_REG back to 0 in the hardware, destroying the configuration we just set? Even though ctrl_save eventually restores the CONTROL_TEST bit in C_CAN_CTRL_REG, the configuration in C_CAN_TEST_REG appears permanently los= t. Should c_can_set_bittiming() preserve the CONTROL_TEST bit when writing to C_CAN_CTRL_REG? > /* loopback mode : useful for self-test function */ > - priv->write_reg(priv, C_CAN_CTRL_REG, CONTROL_TEST); > + priv->write_reg(priv, C_CAN_CTRL_REG, CONTROL_TEST | CONTROL_INIT); > priv->write_reg(priv, C_CAN_TEST_REG, TEST_LBACK); --=20 Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260714164839.7711= 23-1-lucas.alves@lumal21.com.br?part=3D1