Re: [PATCH v5] s390/diag204: Add common diag204 interface with caching

[Heiko Carstens <hca@linux.ibm.com>]

On Fri, Nov 06, 2026 at 12:12:54PM +0100, Mete Durlu wrote:
> On 06/11/2026 00:06, Ilya Leoshkevich wrote:
> > > +int diag204_store(void *buf, bool allow_busy)
> > > +{
> > > +    int rc;
> > > +
> > > +    if (!diag204_is_available())
> > > +        return -EOPNOTSUPP;
> > > +    if (!buf)
> > > +        return -EINVAL;
> > > +    rc = 0;
> > > +    if (allow_busy) {
> > > +        if (!mutex_trylock(&diag204_mutex))
> > > +            return -EBUSY;
> > > +    } else {
> > > +        if (mutex_lock_killable(&diag204_mutex))
> > > +            return -EINTR;
> > > +    }
> > > +    if (diag204_is_cache_valid())
> > > +        goto copy;
> > > +    rc = diag204_update_cache(allow_busy);
> > > +    if (rc)
> > > +        goto out;
> > > +copy:
> > > +    memcpy(buf, diag204_cache_info.buf, diag204_get_buf_size());
> > 
> > I wonder if it would make sense to return the buffer address and keep
> > the lock. Most callers should not take more time than memcpy() and would
> > benefit from simplified buffer management. Others can do memcpy()
> > themselves. __acquires() and __releases() annotations should help with
> > keeping such solution maintainable.
> 
> IMO keeping this self contained would keep the lock management
> burden away from the callers while also preventing duplicate
> lock release chunks and logic.
> __acquires() and __releases() should help indicate any misuse
> but I am not sure if it's worth it.
> One objective of this patch series is to simplify the diag204
> interface, so I would like to keep moving forward with the current
> approach if there is no big reason against it.

If you rework to a common diag204 interface, then in a way that improves it,
please. To be honest, I don't see that here. "allow_busy" got new semantics:
even if there is concurrent reader(!) of diag204_store() the caller will now
get -EBUSY - that doesn't make any sense to me.

We should end up the other way: first like Ilya said: return a pointer to a
locked buffer, that the caller can either copy or read from (make sure that
callers don't sleep (e.g. copy_to_user()) while accessing data. Then drop the
lock, when finished. Sounds like an rw semaphore would be the way to go, since
that allows for concurrent readers, which we will very likely end up with in
the future.

Furthermore updates of the buffer should happen concurrently by e.g. a
worker. _Only_ the worker should know and take care of the "bif" flag and
schedule itself again if busy was indicated. The worker could allocate a
second buffer, write into it, and replace the "global" buffer pointer while
having exclusive access to the semaphore (down_write()).

Readers should still get the old data, while the worker is not finished - if
there is no old data: add readers to a waitqueue, woken up by the worker, when
it is finished, etc... you get the basic idea. Callers of diag204 must _not_
specify anymore if "allow_busy" is ok or not - simplify that interface and
remove allow_busy handling completely from callers.

There are obviouly open points - up to you to solve them :)