[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <13d0a0db-e113-42c4-9fbe-74ebfa46f46b@gmail.com>
Date: Tue, 2 Jul 2024 14:00:05 -0500
From: stuart hayes <stuart.w.hayes@...il.com>
To: Christoph Hellwig <hch@....de>
Cc: linux-kernel@...r.kernel.org,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
"Rafael J . Wysocki" <rafael@...nel.org>,
Martin Belanger <Martin.Belanger@...l.com>,
Oliver O'Halloran <oohall@...il.com>, Daniel Wagner <dwagner@...e.de>,
Keith Busch <kbusch@...nel.org>, Lukas Wunner <lukas@...ner.de>,
David Jeffery <djeffery@...hat.com>, Jeremy Allison <jallison@....com>,
Jens Axboe <axboe@...com>, Sagi Grimberg <sagi@...mberg.me>,
linux-nvme@...ts.infradead.org
Subject: Re: [PATCH v7 3/4] driver core: shut down devices asynchronously
On 7/2/2024 12:04 AM, Christoph Hellwig wrote:
> On Mon, Jul 01, 2024 at 12:57:40PM -0500, stuart hayes wrote:
>>> We discussed this before, but there is no summary of it and I of course
>>> forgot the conclusion:
>>>
>>> - why don't we do this by default?
>>
>> It is done by default in this version, for devices whose drivers opt-in.
>>
>> In the previous discussion, you mentioned that you thought "safe" was the
>> only sensible option (where "safe" was driver opt-in to async shutdown)...
>> that is the default (and only) option with this version. Greg K-H also
>> requested opt-in as well, and suggested that "on" (driver opt-out) could
>> be removed.
>>
>>> - why is it safe to user enable it?
>>
>> I guess it isn't necessarily safe, if there are any drivers that can't
>> handle their devices shutting down asynchronously. I thought it would be
>> nice to be able to enable driver opt-in from user space for testing, before
>> changing the default setting for the driver.
>
> I was mostly getting into the contradiction that either we think the
> async shutdown is safe everywhere, in which case we don't need a driver
> opt-in, or it is not, in which case allowing user to just enabled it
> also seems like a bad idea.
>
I understand. My thinking was that is was very likely to be safe (the initial
version of this patch didn't have an opt-in or opt-out).
I have no issue removing the sysfs attribute if you think that's best.
>> I can correct these lines. I thought that an 80 character line length limit
>> was no longer required, and saw another line a few lines above these that was
>> even longer... and the checkpatch script didn't flag it either.
>
> checkpatch is unfortunately completely broken, it flags totally harmless
> things and doesn't catch other things. > 80 characters are allowed for
> individual lines where it improves readability. The exact definition
> of that depends on the maintainers and reviewers, but outside of
> string constants I can't really find anything where it does.
Got it, thanks for the feedback.
Powered by blists - more mailing lists