From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.remote-ip=95.215.58.14 ARC-Seal: i=1; d=subspace.kernel.org; s=arc20261015; a=rsa-sha256; cv=none; t=1794228021; b=HgQ6RwvFZhlkPqZWdb8Q37lOAw+pDX+CW/Wv5J3S6s2vYVMFcKhfazIoGAn0WWXm378B sD+lFGJl/m/8P9Oj81pbAYVVKSjDv5sd3sAyuN4OGHmN6dqhrF2jYHPySVEeyG/dchlv1 KEP6bI0BBO8bboKCcL2h/2jwxSZVe7DAGsEL1AiIcjVSJReIPtZVLzLokSdnQJcqP2EK0 q7022v4Ppy6pLTeRfWENZTfvzeMWcvjuDmMT8XQJnD0ogCgPtEPxMsbIGcHsMWt4IRIew JLOLOrJP6bG9b+7+Bs51qLuwPwlIOL6aRTwgja0L1DtzEwjGRhziVKwQ0nfA1A7AHuw== ARC-Message-Signature: i=1; d=subspace.kernel.org; s=arc20261015; a=rsa-sha256; c=relaxed/relaxed; t=1794228021; h=DMARC-Filter:X-Envelope-To:DKIM-Signature:X-Envelope-To:Received: X-Mizu-Trace-ID:X-Migadu-Flow:Message-ID:Date:MIME-Version:User-Agent: Subject:To:Cc:References:Content-Language:From:In-Reply-To: Content-Type:Content-Transfer-Encoding; bh=E8rh+A8iUf9NcUV78g/qTQWXpMrMp374iBAZJigpqcI=; b=LREUWiG9Vf/9HO/YHh6cTpHHOsLDelE7INKg0UW/BDzTsp07emG+Wco2A+mzTyjchiZs Km+Wc/xouOZWdeDIk4GjnMAdbJ2alnH+AP1OLjdewtkDhPgdWJ8jIYmi1iC606Xucch/7 Nn0LIJUjJpgCO+k8+i49Qy4mwxPCs1YhLekxJL6d/dFhKvN1BcscpBnr2E1/MfIuxRo5c MfEG4FYTNHhsg/RpeVsyrJd+9UBsIKYjeKT6UFca4/ZliRtMNYMNh0Pl/MwEYWKDqxDOb RE5mnRwjHd7VHi9FmaEBUpA0fDvywCmjO+aaOHUy/MykqmWx8vBfVpCtYX10i59blTA== ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass header.from=linux.dev; spf=pass smtp.mailfrom=linux.dev; dkim=pass header.d=linux.dev header.i=@linux.dev header.a=rsa-sha256 header.s=key1 header.b=IXpQjYCc; arc=none smtp.remote-ip=95.215.58.14 Received: from mta1.migadu.com (out-14.mta1.migadu.com [95.215.58.14]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id B7271606A322 for ; Mon, 9 Nov 2026 12:40:19 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.2 smtp.subspace.kernel.org B7271606A322 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linux.dev Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key, unprotected) header.d=linux.dev header.i=@linux.dev header.a=rsa-sha256 header.s=key1 header.b=IXpQjYCc X-Envelope-To: linux-kernel@vger.kernel.org DKIM-Signature: a=rsa-sha256; bh=AKZh4nPPHH+Z+tE3EwhpBYD5fzfCHy+nEnXWG119NSE=; c=simple/simple; d=linux.dev; h=from:to:subject:date:message-id:mime-version:content-type; s=key1; t=1794228017; v=1; x=1794832817; b=IXpQjYCcDFyFubs7q5Cm2oZ88M/Kw3pGcxyHIhIVF0rq3C+ucPwERlz2AVyE4kkUMXNIF/ZK c7fN9xOQ7EHaIOp1kmmzkSHzcWarnANbzsQepXVgy3MTOHjlvD0Rcp87g4nzctQRKibUfsbbMkr 4gskPFnzqbldwYMKkMy/8AAA= X-Envelope-To: linux-kernel@vger.kernel.org Received: by smtp.migadu.com with ESMTPS id 3d236e2422131cd7; Mon, 09 Nov 2026 12:40:16 +0000 X-Mizu-Trace-ID: 3d236e2422131cd7 X-Migadu-Flow: FLOW_OUT Message-ID: Date: Mon, 9 Nov 2026 12:40:14 +0000 X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Post: List-Help: List-Subscribe: List-Unsubscribe: List-Owner: Precedence: list MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH can-next v5 6/6] can: gs_usb: fix hardware timestamp state for mixed channels To: Marc Kleine-Budde , Vincent Mailhol , Peter Fink , Eric Evenchick , Maximilian Schneider , Wolfgang Grandegger Cc: kernel@pengutronix.de, linux-can@vger.kernel.org, linux-kernel@vger.kernel.org, Shuangpeng Bai , stable@vger.kernel.org References: <20261108-gs_usb-fixes-v5-0-d52dd4940d46@pengutronix.de> <20261108-gs_usb-fixes-v5-6-d52dd4940d46@pengutronix.de> Content-Language: en-US From: Vadim Fedorenko In-Reply-To: <20261108-gs_usb-fixes-v5-6-d52dd4940d46@pengutronix.de> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 08/11/2026 20:47, Marc Kleine-Budde wrote: > From: Shuangpeng Bai > > For hardware timestamping, the driver manages the > infrastructure (cyclecounter/timecounter) for the entire USB interface, > assuming that all channels share a common time base, even though each > channel individually indicates support for hardware timestamping via the > GS_CAN_FEATURE_HW_TIMESTAMP flag. > > Currently both initialization and shutdown of the HW timestamping > infrastructure is tied to the flags of the first channel opened > respectively closed. This leads to problems if the first channel opened > doesn't have the timestamping feature, but the second channel has. > > Keep the initialization of the HW timestamping infrastructure in > gs_usb_timestamp_init(), and move initial time init and the initial > scheduling of the delayed worker into gs_usb_timestamp_start(). > > Initialize the HW timestamping infrastructure in gs_usb_probe() using > gs_usb_timestamp_init() if at least one channel has the > GS_CAN_FEATURE_HW_TIMESTAMP flag set. > > Extend the functions gs_usb_timestamp_start()/gs_usb_timestamp_stop() > to track the number of active timestamp channels. Enabling or disabling the > delayed worker prevents concurrent rescheduling while > gs_usb_timestamp_stop() is running. > > Call gs_usb_timestamp_start()/gs_usb_timestamp_stop() based on the > channel's GS_CAN_FEATURE_HW_TIMESTAMP flag, rather than relying on the > number of active channels. > > Reported-by: Shuangpeng Bai > Closes: https://sashiko.dev/#/bug/linux-b39360f9-6cd8-4f47-83bb-f0f78331f308 > Closes: https://sashiko.dev/#/bug/linux-5f8ecd4e-344e-4ceb-9b46-886f4942a5f6 > Fixes: 5886e4d5ecec ("can: gs_usb: fix time stamp counter initialization") > Cc: stable@vger.kernel.org > Cc: Shuangpeng Bai > Cc: Vadim Fedorenko > Signed-off-by: Shuangpeng Bai > Co-developed-by: Marc Kleine-Budde > Signed-off-by: Marc Kleine-Budde > --- > Changes in v4: > - switch to enable_delayed_work()/disable_delayed_work_sync() > - move starting from gs_usb_timestamp_init() into gs_usb_timestamp_start() > - polish ref-counting/locking > - Link to v3: https://patch.msgid.link/20260720184042.1712217-1-shuangpeng.kernel@gmail.com/ > > Changes in v3: > - Drop Suggested-by tag. > - Move active_timestamp_channels management into the timestamp init/stop > helpers. > - Protect active_timestamp_channels with tc_lock, and keep it zero until > timecounter_init() has completed. > - Make RX timestamp conversion and the delayed work skip the timecounter > while no timestamp-capable channel is active. > - Initialize the shared timestamp lock and delayed work once during probe. > > Changes in v2: > - Count active timestamp-capable channels instead of tracking only whether > the shared timestamp worker has been started. > - Stop the shared timestamp worker when the last timestamp-capable channel > closes, even if non-timestamp channels remain open. > - Unwind the timestamp-capable channel count on gs_can_open() failures. > > --- > > Picking up Shuangpeng Bai's work, I've ported it on top of other fixes for > problems identified by sashiko. > > Marc > --- > drivers/net/can/usb/gs_usb.c | 54 +++++++++++++++++++++++++++++++------------- > 1 file changed, 38 insertions(+), 16 deletions(-) > > diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c > index 6327069f63c9..0a1cd14c940c 100644 > --- a/drivers/net/can/usb/gs_usb.c > +++ b/drivers/net/can/usb/gs_usb.c > @@ -337,6 +337,7 @@ struct gs_usb { > > unsigned int hf_size_rx; > u8 active_channels; > + u8 active_timestamp_channels; > u8 channel_cnt; > > unsigned int pipe_in; > @@ -481,18 +482,35 @@ static void gs_usb_timestamp_init(struct gs_usb *parent) > cc->mult = clocksource_hz2mult(GS_USB_TIMESTAMP_TIMER_HZ, cc->shift); > > spin_lock_init(&parent->tc_lock); > - spin_lock_bh(&parent->tc_lock); > - timecounter_init(&parent->tc, &parent->cc, ktime_get_real_ns()); > - spin_unlock_bh(&parent->tc_lock); > > INIT_DELAYED_WORK(&parent->timestamp, gs_usb_timestamp_work); > + disable_delayed_work(&parent->timestamp); > +} > + > +static void gs_usb_timestamp_start(struct gs_usb *parent) > +{ > + scoped_guard(spinlock_bh, &parent->tc_lock) { > + parent->active_timestamp_channels++; > + if (parent->active_timestamp_channels > 1) > + return; > + > + timecounter_init(&parent->tc, &parent->cc, ktime_get_real_ns()); > + } > + > + enable_delayed_work(&parent->timestamp); > schedule_delayed_work(&parent->timestamp, > GS_USB_TIMESTAMP_WORK_DELAY_SEC * HZ); > } > > static void gs_usb_timestamp_stop(struct gs_usb *parent) > { > - cancel_delayed_work_sync(&parent->timestamp); > + scoped_guard(spinlock_bh, &parent->tc_lock) { > + parent->active_timestamp_channels--; > + if (parent->active_timestamp_channels > 0) > + return; > + } > + > + disable_delayed_work_sync(&parent->timestamp); > } That's quite defensive code. You have both disable/enable delayed_work and active_timestamp_channels counter just to be sure to schedule timestamp worker only once. Just wonder cannot we keep this worker running regardless of channels?