[PATCH v3 linux dev-4.13 2/3] fsi/sbefifo: Add driver for the SBE FIFO

[Nicholas Piggin]

On Mon, 21 May 2018 18:30:40 +1000
Benjamin Herrenschmidt <benh at kernel.crashing.org> wrote:

> +Nick (see question about iovec)
> 
> > > --- /dev/null
> > > +++ b/drivers/fsi/fsi-sbefifo.c
> > > @@ -0,0 +1,867 @@
> > > +/*
> > > + * Copyright (C) IBM Corporation 2017
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify
> > > + * it under the terms of the GNU General Public License version 2 as
> > > + * published by the Free Software Foundation.
> > > + *
> > > + * This program is distributed in the hope that it will be useful,
> > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > + * MERGCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > + * GNU General Public License for more details.
> > > + */  
> > 
> > Should be SPDX-style?  
> 
> Yup, keep forgetting about that new thing :)
> 
> > >   
> 
>  .../...
> 
> > > +static int sbefifo_get_sbe_state(struct sbefifo *sbefifo)  
> > 
> > This isn't so much getting the state as ensuring it's in a usable
> > state, or it's conflating error codes with the SBE state. Maybe
> > sbefifo_is_ready()?  
> 
> Hrm... I would expect something called is_something() to return a
> boolean, while here I have 3 different result codes as I want to
> differenciate busy. I'm also adding some more in the next version
> to extract the bit indicating that an SBE async FFDC is present.
> 
> So what about calling it sbefifo_check_sbe_state() instead ?
> 
> IE. It's not a "getter" .. 
> 
>   .../...
> 
> > > +/**
> > > + * sbefifo_submit() - Submit and SBE fifo command and receive response
> > > + * @dev: The sbefifo device
> > > + * @command: The raw command data
> > > + * @cmd_len: The command size (in 32-bit words)
> > > + * @response: The output response buffer
> > > + * @resp_len: In: Response buffer size, Out: Response size
> > > + *
> > > + * This will perform the entire operation. If the reponse buffer
> > > + * overflows, returns -EOVERFLOW
> > > + */
> > > +int sbefifo_submit(struct device *dev, const __be32 *command, size_t 
> > > cmd_len,
> > > +		   __be32 *response, size_t *resp_len)
> > > +{
> > > +	struct sbefifo *sbefifo = dev_get_drvdata(dev);
> > > +	mm_segment_t old_fs;
> > > +	int rc;
> > > +
> > > +	if (!dev || !sbefifo)
> > > +		return -ENODEV;
> > > +	if (WARN_ON_ONCE(sbefifo->magic != SBEFIFO_MAGIC))
> > > +		return -ENODEV;
> > > +	if (!resp_len || !command || !response || cmd_len > 
> > > SBEFIFO_MAX_CMD_LEN)
> > > +		return -EINVAL;
> > > +
> > > +	mutex_lock(&sbefifo->lock);
> > > +	old_fs = get_fs();
> > > +	set_fs(KERNEL_DS);
> > > +	rc = __sbefifo_submit(sbefifo, command, cmd_len,
> > > +			      response, resp_len);
> > > +	set_fs(old_fs);  
> > 
> > Can you talk about why the get_fs()/set_fs()/set_fs() sequence is the
> > right way to go here when there's been discussion in the past about
> > removing the whole show?
> > 
> > 

> Interesting, I had missed that conversation/debate, it's an old API but
> I've been around for a long time :-) It used to be the way to do that
> sort of thing.
> 
> The above use is a rather safe since the oops case has been fixed, but
> if it's actively being phased out then I should try to find a different
> way, though sadly nothing will be as simple and efficient as the above.
> 
> Nick, you added some of that iovec stuff, what do you think is the
> right construct here ?

Yeah I don't know that there's strict policy against set_fs now...
anything is a security bug if you write it with security bugs :)
Seems like there's still quite a lot of users of it. I would say
maybe it's okay to use set_fs in this case because it's all contained
in the driver so we're not talking about new interfaces added to the
kernel. Is there any prohibition on adding new users?

> 
> I read from the HW fifo synchronously and currently just put_user() one
> word at a time into the destination buffer, which can be either a user
> address (read() syscall to the driver) or a kernel address (in-kernel
> API).
> 
> The old code just had 2 pointer argument, one kernel pointer and one
> user pointer and would write to which ever was populated, which I
> thought was gross.
> 
> The iovec stuff looks ... complicated for such a simple use case.

It shouldn't be _too_ hard to use. I think for your code just do an
iov_iter_init or iov_iter_kvec with 1 vec depending on whether it's
user or kernel, and pass that iter into __submit and copy_to_iter.

        /* kernel */
        struct kvec iov = {.iov_base = response, .iov_len = resp_len};
        struct iov_iter resp;
        iov_iter_kvec(&resp, WRITE | ITER_KVEC, &iov, 1, rlen);

        /* user */
        struct iovec iov = { .iov_base = buf, .iov_len = rlen};
        struct iov_iter resp;
        iov_iter_init(&resp, WRITE, &iov, 1, rlen);

Thanks,
Nick