On Thu, 8 Aug 2024 19:31:57 -0700 Allen wrote:

quoted

quoted

In the context of of the driver, the conversion from tasklet_enable()
to enable_and_queue_work() is correct because the callback function
associated with the work item is designed to be safe even if there
is no immediate work to process. The callback function can handle
being invoked in such situations without causing errors or undesirable
behavior. This makes the workqueue approach a suitable and safe
replacement for the current tasklet mechanism, as it provides the
necessary flexibility and ensures that the work item is properly
scheduled and executed.  

Fewer words, clearer indication that you read the code would be better
for the reviewer. Like actually call out what in the code makes it safe.
 

Okay.

quoted

Just to be clear -- conversions to enable_and_queue_work() will require
manual inspection in every case.  

Attempting again.

The enable_and_queue_work() only schedules work if it is not already
enabled, similar to how tasklet_enable() would only allow a tasklet to run
if it had been previously scheduled.

In the current driver, where we are attempting conversion, enable_work()
checks whether the work is already enabled and only enables it if it
was disabled. If no new work is queued, queue_work() won't be called.
Hence, the callback is safe even if there's no work.

Hm. Let me give you an example of what I was hoping to see for this
patch (in addition to your explanation of the API difference):

 The conversion for oct_priv->droq_bh_work should be safe. While
 the work is per adapter, the callback (octeon_droq_bh()) walks all
 queues, and for each queue checks whether the oct->io_qmask.oq mask
 has a bit set. In case of spurious scheduling of the work - none of
 the bits should be set, making the callback a noop.