linux-kernel - Re: [PATCH] drm/sun4i: tcon: Support keeping dclk rate upon ancestor clock changes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87le6pcfoq.fsf@oltmanns.dev>
Date: Mon, 11 Mar 2024 09:53:41 +0100
From: Frank Oltmanns <frank@...manns.dev>
To: Ondřej Jirman <x@...x.eu>
Cc: Maxime Ripard <mripard@...nel.org>,  Chen-Yu Tsai <wens@...e.org>,
  Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,  Thomas Zimmermann
 <tzimmermann@...e.de>,  David Airlie <airlied@...il.com>,  Daniel Vetter
 <daniel@...ll.ch>,  Jernej Skrabec <jernej.skrabec@...il.com>,  Samuel
 Holland <samuel@...lland.org>,  Icenowy Zheng <uwu@...nowy.me>,
  dri-devel@...ts.freedesktop.org,  linux-arm-kernel@...ts.infradead.org,
  linux-sunxi@...ts.linux.dev,  linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm/sun4i: tcon: Support keeping dclk rate upon
 ancestor clock changes

Hello Ondřej,

On 2024-03-10 at 23:23:57 +0100, Ondřej Jirman <x@...x.eu> wrote:
> Hello Frank,
>
> On Sun, Mar 10, 2024 at 02:32:29PM +0100, Frank Oltmanns wrote:
>> +static int sun4i_rate_reset_notifier_cb(struct notifier_block *nb,
>> +				      unsigned long event, void *data)
>> +{
>> +	struct sun4i_rate_reset_nb *rate_reset = to_sun4i_rate_reset_nb(nb);
>> +
>> +	if (event == POST_RATE_CHANGE)
>> +		schedule_delayed_work(&rate_reset->reset_rate_work, msecs_to_jiffies(100));
>
> If you get multiple reset notifier calls within 100ms of the first one,
> the delay from the last one will not be 100ms, so this may violate expectations
> you're describing in the commit message.

Let me start by saying, the implicit expectation is that the new user of
pll-video0 will get an exclusive lock on pll-video0 (otherwise,
data-clock would reset pll-video0 after those 100ms rendering the whole
endeavor of the new user pointless). This constraint makes the chances
that the above event (two consecutive rate changes within 100ms) occurs
very slim.

That being said, I don't see a problem with cancelling the delayed work
on PRE_RATE_CHANGE and restarting it on ABORT_RATE_CHANGE like this:

+static int sun4i_rate_reset_notifier_cb(struct notifier_block *nb,
+				      unsigned long event, void *data)
+{
+	struct sun4i_rate_reset_nb *rate_reset = to_sun4i_rate_reset_nb(nb);
+
+	if (event == PRE_RATE_CHANGE) {
+		rate_reset->is_cancelled = cancel_delayed_work(&rate_reset->reset_rate_work);
+	} else if ((event == POST_RATE_CHANGE) ||
+		 (event == ABORT_RATE_CHANGE) && rate_reset->is_cancelled) {
+		schedule_delayed_work(&rate_reset->reset_rate_work, msecs_to_jiffies(100));
+		rate_reset->is_cancelled = false;
+	}
+
+	return NOTIFY_DONE;
+}

The need for this new code is slim (IMHO) but on the other hand it
doesn't add much complexity. I'll add it in V2 including the
is_cancelled member in sun4i_rate_reset_nb if I don't receive any
objections during this week.

Thanks,
  Frank

>
> schedule_delayed_work doesn't re-schedule the work if it's already pending.
>
> Kind regards,
> 	o.