* Re: [PATCH v6 09/15] dt-bindings: Document the STM32 USART bindings
From: Rob Herring @ 2015-04-07 19:26 UTC (permalink / raw)
To: Maxime Coquelin
Cc: Uwe Kleine-König, Andreas Färber, Geert Uytterhoeven,
Rob Herring, Philipp Zabel, Linus Walleij, Arnd Bergmann,
Stefan Agner, Peter Meerwald, Paul Bolle, Peter Hurley,
Andy Shevchenko, Chanwoo Choi, Russell King, Daniel Lezcano,
Joe Perches, Jonathan Corbet, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Thomas Gleixner
In-Reply-To: <1428424234-28572-10-git-send-email-mcoquelin.stm32@gmail.com>
On Tue, Apr 7, 2015 at 11:30 AM, Maxime Coquelin
<mcoquelin.stm32@gmail.com> wrote:
> This adds documentation of device tree bindings for the
> STM32 USART
>
> Tested-by: Chanwoo Choi <cw00.choi@samsung.com>
> Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>
Acked-by: Rob Herring <robh@kernel.org>
> ---
> .../devicetree/bindings/serial/st,stm32-usart.txt | 32 ++++++++++++++++++++++
> 1 file changed, 32 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/serial/st,stm32-usart.txt
>
> diff --git a/Documentation/devicetree/bindings/serial/st,stm32-usart.txt b/Documentation/devicetree/bindings/serial/st,stm32-usart.txt
> new file mode 100644
> index 0000000..8480a76
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/serial/st,stm32-usart.txt
> @@ -0,0 +1,32 @@
> +* STMicroelectronics STM32 USART
> +
> +Required properties:
> +- compatible: Can be either "st,stm32-usart" or "st,stm32-uart" depending on
> +whether the device supports synchronous mode.
> +- reg: The address and length of the peripheral registers space
> +- interrupts: The interrupt line of the USART instance
> +- clocks: The input clock of the USART instance
> +
> +Optional properties:
> +- pinctrl: The reference on the pins configuration
> +- auto-flow-control: bool flag to enable hardware flow control.
> +
> +Examples:
> +usart4: serial@40004c00 {
> + compatible = "st,stm32-uart";
> + reg = <0x40004c00 0x400>;
> + interrupts = <52>;
> + clocks = <&clk_pclk1>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_usart4>;
> +};
> +
> +usart2: serial@40004400 {
> + compatible = "st,stm32-usart", "st,stm32-uart";
> + reg = <0x40004400 0x400>;
> + interrupts = <38>;
> + clocks = <&clk_pclk1>;
> + auto-flow-control;
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_usart2 &pinctrl_usart2_rtscts>;
> +};
> --
> 1.9.1
>
^ permalink raw reply
* Re: [PATCH v4 05/10] eeprom: Add bindings for simple eeprom framework
From: Matt Porter @ 2015-04-07 19:46 UTC (permalink / raw)
To: Srinivas Kandagatla
Cc: Mark Brown, Rob Herring, linux-arm-kernel@lists.infradead.org,
devicetree@vger.kernel.org, Arnd Bergmann, Greg Kroah-Hartman,
Sascha Hauer, Stephen Boyd, linux-kernel@vger.kernel.org,
Rob Herring, Kumar Gala, Maxime Ripard, linux-api@vger.kernel.org,
linux-arm-msm
In-Reply-To: <55241BF2.4050609@linaro.org>
On Tue, Apr 07, 2015 at 07:03:30PM +0100, Srinivas Kandagatla wrote:
>
>
> On 07/04/15 18:46, Mark Brown wrote:
> >On Tue, Apr 07, 2015 at 06:35:49PM +0100, Srinivas Kandagatla wrote:
> >>On 06/04/15 16:04, Matt Porter wrote:
> >>>On Mon, Apr 06, 2015 at 09:11:05AM -0500, Rob Herring wrote:
> >
> >>>>>The generic binding could really use a "read-only" property here as this
> >>>>>is a common hardware attribute for many nvmem devices. A serial EEPROM
> >
> >>Correct me If am wrong.
> >
> >>Regarding write protection/read-only, regmap already has provisions to
> >>support this feature. regmap would bail out with errors if any attempt to
> >>write to non-writable regions. It all depends on the data providers how they
> >>setup the regmap and the bindings for those are specific individual data
> >>providers I think.
> >
> >There is the ability to flag read/write permissions in regmap but I
> >think there's some suggestion that this should be exposed to userspace
> >so that it's easier for it to handle things rather than just writing
> >then coping with any errors.
>
> Yes, That's possible if the data provider use the "read-only" generic
> binding like MTD partitions which the eeprom framwork could use to set the
> binary file mode appropriately.
Right, that's what Rob suggested as to how it should be exposed to
userspace. I think Mark is suggesting that it can also be done by
returning appropriately fine-grained error codes from a writeable
attribute.
Just to clarify here, I brought this up because if we only allow
the writes to fail, there's not necessarily not enough information
available to know if they failed due to a real error (perhaps write
cycles for the underlying nvmem device have been exhausted) or is
it simply that the underlying device has been hardware write protected
(such as as the write protect pin hardwired that way or it's an OTP
device). The client, whether userspace or otherwise is going to need
to know this information to make informed policy decisions.
-Matt
> "read-only" property seems to be more generic for all types of data
> providers.
>
> I will give it a try and document this in the bindings too in next version.
>
> --srini
> >
^ permalink raw reply
* Re: [PATCH v4 04/10] eeprom: Add a simple EEPROM framework for eeprom consumers
From: Srinivas Kandagatla @ 2015-04-07 20:09 UTC (permalink / raw)
To: Stephen Boyd
Cc: linux-arm-kernel, Maxime Ripard, Rob Herring, Kumar Gala,
Mark Brown, s.hauer, Greg Kroah-Hartman, linux-api, linux-kernel,
devicetree, linux-arm-msm, arnd
In-Reply-To: <20150407184533.GA10278@codeaurora.org>
Thanks Stephen for review,
On 07/04/15 19:45, Stephen Boyd wrote:
> On 03/30, Srinivas Kandagatla wrote:
>> @@ -130,6 +138,37 @@ static struct class eeprom_class = {
>> .dev_release = eeprom_release,
>> };
>>
>> +static int of_eeprom_match(struct device *dev, const void *eeprom_np)
>> +{
>> + return dev->of_node == eeprom_np;
>> +}
>> +
>> +static struct eeprom_device *of_eeprom_find(struct device_node *eeprom_np)
>> +{
>> + struct device *d;
>> +
>> + if (!eeprom_np)
>> + return NULL;
>> +
>> + d = class_find_device(&eeprom_class, NULL, eeprom_np, of_eeprom_match);
>> +
>> + return d ? to_eeprom(d) : NULL;
>> +}
>> +
>> +static int eeprom_match(struct device *dev, const void *data)
>> +{
>> + return !strcmp(dev_name(dev), (const char *)data);
>
> Is this cast necessary?
May be an over do here, I will fix it.
>
>> +}
>> +
>> +static struct eeprom_device *eeprom_find(const char *name)
>> +{
>> + struct device *d;
>> +
>> + d = class_find_device(&eeprom_class, NULL, (void *)name, eeprom_match);
>
> Is this cast necessary?
I Will fix it in next version.
>
>> +
>> + return d ? to_eeprom(d) : NULL;
>> +}
>> +
>> /**
>> * eeprom_register(): Register a eeprom device for given eeprom.
>> * Also creates an binary entry in /sys/class/eeprom/name-id/eeprom
>> +
>> +/**
>> + * eeprom_cell_get(): Get eeprom cell of device form a given eeprom name
>
> s/form/from/
Will fix this in next version.
>
>> + * and blocks.
>> + *
>> + * @ename: eeprom device name that needs to be looked-up.
>> + * @blocks: eeprom blocks containing offset and length information.
>> + * @nblocks: number of eeprom blocks.
>> + *
>> + * The return value will be an ERR_PTR() on error or a valid pointer
>> + * to a struct eeprom_cell. The eeprom_cell will be freed by the
>> + * eeprom_cell_put().
>> + */
>> +struct eeprom_cell *eeprom_cell_get(const char *ename,
>> + struct eeprom_block *blocks, int nblocks)
>> +{
>> + return __eeprom_cell_get(NULL, ename, blocks, nblocks);
>> +}
>> +EXPORT_SYMBOL_GPL(eeprom_cell_get);
>> +
>> +/**
>> + * of_eeprom_cell_get(): Get eeprom cell of device form a given index
>
> s/form/from/
>
Ok,
>> + *
>> + * @dev node: Device tree node that uses the eeprom cell
>> +
>> +#ifndef _LINUX_EEPROM_CONSUMER_H
>> +#define _LINUX_EEPROM_CONSUMER_H
>> +
>> +struct eeprom_cell;
>> +
>> +struct eeprom_block {
>> + loff_t offset;
>> + size_t count;
>> +};
>> +#if IS_ENABLED(CONFIG_EEPROM)
>> +struct eeprom_cell *eeprom_cell_get(const char *ename,
>> + struct eeprom_block *blocks, int nblocks);
>> +void eeprom_cell_put(struct eeprom_cell *cell);
>> +char *eeprom_cell_read(struct eeprom_cell *cell, ssize_t *len);
>> +int eeprom_cell_write(struct eeprom_cell *cell, const char *buf, ssize_t len);
> [...]
>> +
>> +#if IS_ENABLED(CONFIG_EEPROM) && IS_ENABLED(CONFIG_OF)
>> +struct eeprom_cell *of_eeprom_cell_get(struct device_node *dev,
>> + const char *property);
>> +#else
>> +static inline struct eeprom_cell *of_eeprom_cell_get(struct device_node *np,
>> + const char *property)
>> +{
>> + return ERR_PTR(-ENOSYS);
>> +}
>> +#endif
>> +#endif /* ifndef _LINUX_EEPROM_CONSUMER_H */
>
> Do you have an overview of how to use these APIs? Maybe some
> Documentation/ is in order? I'm mostly interested in how the
> blocks array is supposed to work and how this hooks up to drivers
> that are using DT.
Only doc ATM is function level kernel doc in c file.
May be I can explain you for now and I will try to add some
documentation with some usage examples in next version.
eeprom block array is just another way intended to get hold of eeprom
content for non-DT providers/consumers, but DT consumers/providers can
also use it. As of today SOC/mach level code could use it as well.
In eeprom_cell_get() case the lookup of provider is done based on
provider name, this provider name is generally supplied by all the
providers (both DT/non DT).
for example in qfprom case,
provider is registered from DT with eeprom config containing a unique name:
static struct eeprom_config econfig = {
.name = "qfprom",
.id = 0,
};
In the consumer case, the tsens driver could do some like in non DT way:
struct eeprom_block blocks[4] ={
{
.offset = 0x404,
.count = 0x4,
},
{
.offset = 0x408,
.count = 0x4,
},
{
.offset = 0x40c,
.count = 0x4,
},
{
.offset = 0x410,
.count = 0x4,
},
};
calib_cell = eeprom_cell_get("qfprom0", blocks, 4);
Or in DT way
calib_cell = of_eeprom_cell_get(np, "calib");
--srini
>
^ permalink raw reply
* Re: [GIT PULL] selftest: Add futex functional tests
From: Darren Hart @ 2015-04-08 4:12 UTC (permalink / raw)
To: Shuah Khan
Cc: Darren Hart, Linux Kernel Mailing List,
linux-api-u79uwXL29TY76Z2rM5mHXA, Ingo Molnar, Peter Zijlstra,
Thomas Gleixner, Davidlohr Bueso, KOSAKI Motohiro,
greg Kroah-Hartman, fengguang.wu-ral2JQCrhuEAvxtiuMwx3w,
Michael Ellerman
In-Reply-To: <551ACD5F.8010003-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org>
On Tue, Mar 31, 2015 at 10:37:51AM -0600, Shuah Khan wrote:
> On 03/31/2015 10:24 AM, Darren Hart wrote:
> > On 3/31/15, 8:32 AM, "Shuah Khan" <shuahkh-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org> wrote:
> >
> >> Hi Daren,
> >>
> >> On 03/27/2015 04:17 PM, Darren Hart wrote:
> >>> Hi Shuah,
> >>>
> >>> This series begins the process of migrating my futextest tests into
> >>> kselftest.
> >>> I've started with only the functional tests, as the performance and
> >>> stress may
> >>> not be appropriate for kselftest as they stand.
> >>>
> >>> I cleaned up various complaints from checkpatch, but I ignored others
> >>> that would
> >>> require significant rework of the testcases, such as not using volatile
> >>> and not
> >>> creating new typedefs.
> >>>
> >>> The patches will follow, but I'm providing a pull request for your
> >>> convenience
> >>> as well.
> >>
> >> Thanks for acting on this so quickly after we talked about it at ELC.
> >> Just a quick note that I am going to get to this soon once I get the
> >> 4.1 content wrapped up. We can plan upon getting these into 4.2.
> >
> > OK. Michael E. provided some feedback which I can either incorporate and
> > respin, or I can send as a follow-on to your -next after you merge these.
> > Which do you prefer?
> >
...
> You can wait to re-do patches. I am planning to review the
> patch set later on this week. That way you can avoid re-spin
> just in case, I have other comments.
Hi Shuah,
Did you have any additional comments for the futex tests?
Thanks,
--
Darren Hart
Intel Open Source Technology Center
^ permalink raw reply
* Re: [PATCH v3 7/7] vhost: feature to set the vring endianness
From: Greg Kurz @ 2015-04-08 7:36 UTC (permalink / raw)
To: Cornelia Huck
Cc: kvm, Michael S. Tsirkin, linux-api, linux-kernel, virtualization
In-Reply-To: <20150407170131.3899d62c.cornelia.huck@de.ibm.com>
On Tue, 7 Apr 2015 17:01:31 +0200
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> On Tue, 07 Apr 2015 14:19:31 +0200
> Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
>
> > This patch brings cross-endian support to vhost when used to implement
> > legacy virtio devices. Since it is a relatively rare situation, the
> > feature availability is controlled by a kernel config option (not set
> > by default).
> >
> > The ioctls introduced by this patch are for legacy only: virtio 1.0
> > devices are returned EPERM.
> >
> > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > ---
> > drivers/vhost/Kconfig | 10 ++++++++
> > drivers/vhost/vhost.c | 55 ++++++++++++++++++++++++++++++++++++++++++++
> > drivers/vhost/vhost.h | 17 +++++++++++++-
> > include/uapi/linux/vhost.h | 5 ++++
> > 4 files changed, 86 insertions(+), 1 deletion(-)
>
> > +#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
> > +static long vhost_set_vring_endian_legacy(struct vhost_virtqueue *vq,
> > + void __user *argp)
> > +{
> > + struct vhost_vring_state s;
> > +
> > + if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
> > + return -EPERM;
> > +
> > + if (copy_from_user(&s, argp, sizeof(s)))
> > + return -EFAULT;
> > +
> > + vq->legacy_is_little_endian = !!s.num;
> > + return 0;
> > +}
> > +
> > +static long vhost_get_vring_endian_legacy(struct vhost_virtqueue *vq,
> > + u32 idx,
> > + void __user *argp)
> > +{
> > + struct vhost_vring_state s = {
> > + .index = idx,
> > + .num = vq->legacy_is_little_endian
> > + };
> > +
> > + if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
> > + return -EPERM;
> > +
> > + if (copy_to_user(argp, &s, sizeof(s)))
> > + return -EFAULT;
> > +
> > + return 0;
> > +}
> > +#else
> > +static long vhost_set_vring_endian_legacy(struct vhost_virtqueue *vq,
> > + void __user *argp)
> > +{
> > + return 0;
>
> I'm wondering whether this handler should return an error if the
> feature is not configured for the kernel? How can the userspace caller
> find out whether it has successfully prompted the kernel to handle the
> endianness correctly?
>
Yes you're right. I think -ENOIOCTLCMD as suggested by Michael is a good
candidate.
Thanks.
> > +}
> > +
> > +static long vhost_get_vring_endian_legacy(struct vhost_virtqueue *vq,
> > + u32 idx,
> > + void __user *argp)
> > +{
> > + return 0;
> > +}
> > +#endif /* CONFIG_VHOST_SET_ENDIAN_LEGACY */
^ permalink raw reply
* Re: [PATCH v3 7/7] vhost: feature to set the vring endianness
From: Greg Kurz @ 2015-04-08 8:23 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: linux-api, linux-kernel, kvm, virtualization
In-Reply-To: <20150407155228.GA1907@redhat.com>
On Tue, 7 Apr 2015 17:52:28 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Tue, Apr 07, 2015 at 02:19:31PM +0200, Greg Kurz wrote:
> > This patch brings cross-endian support to vhost when used to implement
> > legacy virtio devices. Since it is a relatively rare situation, the
> > feature availability is controlled by a kernel config option (not set
> > by default).
> >
> > The ioctls introduced by this patch are for legacy only: virtio 1.0
> > devices are returned EPERM.
> >
> > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
>
> EINVAL probably makes more sense?
>
I had choosen EPERM because the error isn't related to the arguments
being passed by userspace. It simply does not make sense to set the
vring endianness for a virtio 1.0 device.
That being said, I am perfectly fine with EINVAL. :)
> > ---
> > drivers/vhost/Kconfig | 10 ++++++++
> > drivers/vhost/vhost.c | 55 ++++++++++++++++++++++++++++++++++++++++++++
> > drivers/vhost/vhost.h | 17 +++++++++++++-
> > include/uapi/linux/vhost.h | 5 ++++
> > 4 files changed, 86 insertions(+), 1 deletion(-)
> >
> > Changes since v2:
> > - fixed typos in Kconfig description
> > - renamed vq->legacy_big_endian to vq->legacy_is_little_endian
> > - vq->legacy_is_little_endian reset to default in vhost_vq_reset()
> > - dropped VHOST_F_SET_ENDIAN_LEGACY feature
> > - dropped struct vhost_vring_endian from the user API (re-use
> > struct vhost_vring_state instead)
> > - added VHOST_GET_VRING_ENDIAN_LEGACY ioctl
> > - introduced more helpers and stubs to avoid polluting the code with ifdefs
> >
> >
> > diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
> > index 017a1e8..0aec88c 100644
> > --- a/drivers/vhost/Kconfig
> > +++ b/drivers/vhost/Kconfig
> > @@ -32,3 +32,13 @@ config VHOST
> > ---help---
> > This option is selected by any driver which needs to access
> > the core of vhost.
> > +
> > +config VHOST_SET_ENDIAN_LEGACY
> > + bool "Cross-endian support for host kernel accelerator"
> > + default n
> > + ---help---
> > + This option allows vhost to support guests with a different byte
> > + ordering from host. It is disabled by default since it adds overhead
> > + and it is only needed by a few platforms (powerpc and arm).
> > +
> > + If unsure, say "N".
> > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > index 2ee2826..3529a3c 100644
> > --- a/drivers/vhost/vhost.c
> > +++ b/drivers/vhost/vhost.c
> > @@ -199,6 +199,7 @@ static void vhost_vq_reset(struct vhost_dev *dev,
> > vq->call = NULL;
> > vq->log_ctx = NULL;
> > vq->memory = NULL;
> > + vq->legacy_is_little_endian = virtio_legacy_is_little_endian();
> > }
> >
> > static int vhost_worker(void *data)
> > @@ -630,6 +631,54 @@ static long vhost_set_memory(struct vhost_dev *d, struct vhost_memory __user *m)
> > return 0;
> > }
> >
> > +#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
> > +static long vhost_set_vring_endian_legacy(struct vhost_virtqueue *vq,
> > + void __user *argp)
> > +{
> > + struct vhost_vring_state s;
> > +
> > + if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
> > + return -EPERM;
>
> EINVAL probably makes more sense? But I'm not sure this
> is helpful: one can set VIRTIO_F_VERSION_1 afterwards,
> and your patch does not seem to detect this.
>
Yeah, when I dropped the *bogus* feature from v2, I forgot to patch
VHOST_SET_FEATURES accordingly... But thinking about it now, the choice
to error out when setting VIRTIO_F_VERSION_1 because cross-endian legacy
was set before looks terrible... :-\
>
>
> > +
> > + if (copy_from_user(&s, argp, sizeof(s)))
> > + return -EFAULT;
> > +
> > + vq->legacy_is_little_endian = !!s.num;
> > + return 0;
> > +}
> > +
> > +static long vhost_get_vring_endian_legacy(struct vhost_virtqueue *vq,
> > + u32 idx,
> > + void __user *argp)
> > +{
> > + struct vhost_vring_state s = {
> > + .index = idx,
> > + .num = vq->legacy_is_little_endian
> > + };
> > +
> > + if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
> > + return -EPERM;
> > +
> > + if (copy_to_user(argp, &s, sizeof(s)))
> > + return -EFAULT;
> > +
> > + return 0;
> > +}
> > +#else
> > +static long vhost_set_vring_endian_legacy(struct vhost_virtqueue *vq,
> > + void __user *argp)
> > +{
> > + return 0;
> > +}
> > +
> > +static long vhost_get_vring_endian_legacy(struct vhost_virtqueue *vq,
> > + u32 idx,
> > + void __user *argp)
> > +{
> > + return 0;
> > +}
>
> Should be -ENOIOCTLCMD?
>
Sure.
> > +#endif /* CONFIG_VHOST_SET_ENDIAN_LEGACY */
> > +
> > long vhost_vring_ioctl(struct vhost_dev *d, int ioctl, void __user *argp)
> > {
> > struct file *eventfp, *filep = NULL;
> > @@ -806,6 +855,12 @@ long vhost_vring_ioctl(struct vhost_dev *d, int ioctl, void __user *argp)
> > } else
> > filep = eventfp;
> > break;
> > + case VHOST_SET_VRING_ENDIAN_LEGACY:
> > + r = vhost_set_vring_endian_legacy(vq, argp);
> > + break;
> > + case VHOST_GET_VRING_ENDIAN_LEGACY:
> > + r = vhost_get_vring_endian_legacy(vq, idx, argp);
> > + break;
> > default:
> > r = -ENOIOCTLCMD;
> > }
>
> I think we also want to forbid this with a running backend.
>
Yes.
> > diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> > index 4e9a186..981ba06 100644
> > --- a/drivers/vhost/vhost.h
> > +++ b/drivers/vhost/vhost.h
> > @@ -106,6 +106,9 @@ struct vhost_virtqueue {
> > /* Log write descriptors */
> > void __user *log_base;
> > struct vhost_log *log;
> > +
> > + /* We need to know the device endianness with legacy virtio. */
> > + bool legacy_is_little_endian;
> > };
> >
> > struct vhost_dev {
> > @@ -173,11 +176,23 @@ static inline bool vhost_has_feature(struct vhost_virtqueue *vq, int bit)
> > return vq->acked_features & (1ULL << bit);
> > }
> >
> > +#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
> > +static inline bool vhost_legacy_is_little_endian(struct vhost_virtqueue *vq)
> > +{
> > + return vq->legacy_is_little_endian;
> > +}
> > +#else
> > +static inline bool vhost_legacy_is_little_endian(struct vhost_virtqueue *vq)
> > +{
> > + return virtio_legacy_is_little_endian();
> > +}
> > +#endif
> > +
> > static inline bool vhost_is_little_endian(struct vhost_virtqueue *vq)
> > {
> > if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
> > return true;
> > - return virtio_legacy_is_little_endian();
> > + return vhost_legacy_is_little_endian(vq);
> > }
> >
> > /* Memory accessors */
> > diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> > index bb6a5b4..1b01a72 100644
> > --- a/include/uapi/linux/vhost.h
> > +++ b/include/uapi/linux/vhost.h
> > @@ -103,6 +103,11 @@ struct vhost_memory {
> > /* Get accessor: reads index, writes value in num */
> > #define VHOST_GET_VRING_BASE _IOWR(VHOST_VIRTIO, 0x12, struct vhost_vring_state)
> >
> > +/* Set endianness for the ring (legacy virtio only) */
> > +/* num is 0 for big endian, other values mean little endian */
>
> I'd prefer 0 and 1, return EINVAL on other values.
>
Ok.
> > +#define VHOST_SET_VRING_ENDIAN_LEGACY _IOW(VHOST_VIRTIO, 0x13, struct vhost_vring_state)
> > +#define VHOST_GET_VRING_ENDIAN_LEGACY _IOW(VHOST_VIRTIO, 0x14, struct vhost_vring_state)
> > +
> > /* The following ioctls use eventfd file descriptors to signal and poll
> > * for events. */
> >
>
Thanks.
--
Greg
^ permalink raw reply
* Re: [PATCH v3 7/7] vhost: feature to set the vring endianness
From: Greg Kurz @ 2015-04-08 8:25 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: linux-api, linux-kernel, kvm, virtualization
In-Reply-To: <20150407175640-mutt-send-email-mst@redhat.com>
On Tue, 7 Apr 2015 18:11:29 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Tue, Apr 07, 2015 at 02:19:31PM +0200, Greg Kurz wrote:
> > This patch brings cross-endian support to vhost when used to implement
> > legacy virtio devices. Since it is a relatively rare situation, the
> > feature availability is controlled by a kernel config option (not set
> > by default).
> >
> > The ioctls introduced by this patch are for legacy only: virtio 1.0
> > devices are returned EPERM.
> >
> > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > ---
> > drivers/vhost/Kconfig | 10 ++++++++
> > drivers/vhost/vhost.c | 55 ++++++++++++++++++++++++++++++++++++++++++++
> > drivers/vhost/vhost.h | 17 +++++++++++++-
> > include/uapi/linux/vhost.h | 5 ++++
> > 4 files changed, 86 insertions(+), 1 deletion(-)
> >
> > Changes since v2:
> > - fixed typos in Kconfig description
> > - renamed vq->legacy_big_endian to vq->legacy_is_little_endian
> > - vq->legacy_is_little_endian reset to default in vhost_vq_reset()
> > - dropped VHOST_F_SET_ENDIAN_LEGACY feature
> > - dropped struct vhost_vring_endian from the user API (re-use
> > struct vhost_vring_state instead)
> > - added VHOST_GET_VRING_ENDIAN_LEGACY ioctl
> > - introduced more helpers and stubs to avoid polluting the code with ifdefs
> >
> >
> > diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
> > index 017a1e8..0aec88c 100644
> > --- a/drivers/vhost/Kconfig
> > +++ b/drivers/vhost/Kconfig
> > @@ -32,3 +32,13 @@ config VHOST
> > ---help---
> > This option is selected by any driver which needs to access
> > the core of vhost.
> > +
> > +config VHOST_SET_ENDIAN_LEGACY
> > + bool "Cross-endian support for host kernel accelerator"
> > + default n
> > + ---help---
> > + This option allows vhost to support guests with a different byte
> > + ordering from host. It is disabled by default since it adds overhead
> > + and it is only needed by a few platforms (powerpc and arm).
> > +
> > + If unsure, say "N".
> > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > index 2ee2826..3529a3c 100644
> > --- a/drivers/vhost/vhost.c
> > +++ b/drivers/vhost/vhost.c
> > @@ -199,6 +199,7 @@ static void vhost_vq_reset(struct vhost_dev *dev,
> > vq->call = NULL;
> > vq->log_ctx = NULL;
> > vq->memory = NULL;
> > + vq->legacy_is_little_endian = virtio_legacy_is_little_endian();
> > }
> >
> > static int vhost_worker(void *data)
> > @@ -630,6 +631,54 @@ static long vhost_set_memory(struct vhost_dev *d, struct vhost_memory __user *m)
> > return 0;
> > }
> >
> > +#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
> > +static long vhost_set_vring_endian_legacy(struct vhost_virtqueue *vq,
> > + void __user *argp)
> > +{
> > + struct vhost_vring_state s;
> > +
> > + if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
> > + return -EPERM;
> > +
> > + if (copy_from_user(&s, argp, sizeof(s)))
> > + return -EFAULT;
> > +
> > + vq->legacy_is_little_endian = !!s.num;
> > + return 0;
> > +}
> > +
> > +static long vhost_get_vring_endian_legacy(struct vhost_virtqueue *vq,
> > + u32 idx,
> > + void __user *argp)
> > +{
> > + struct vhost_vring_state s = {
> > + .index = idx,
> > + .num = vq->legacy_is_little_endian
> > + };
> > +
> > + if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
> > + return -EPERM;
> > +
> > + if (copy_to_user(argp, &s, sizeof(s)))
> > + return -EFAULT;
> > +
> > + return 0;
> > +}
> > +#else
> > +static long vhost_set_vring_endian_legacy(struct vhost_virtqueue *vq,
> > + void __user *argp)
> > +{
> > + return 0;
> > +}
> > +
> > +static long vhost_get_vring_endian_legacy(struct vhost_virtqueue *vq,
> > + u32 idx,
> > + void __user *argp)
> > +{
> > + return 0;
> > +}
> > +#endif /* CONFIG_VHOST_SET_ENDIAN_LEGACY */
> > +
> > long vhost_vring_ioctl(struct vhost_dev *d, int ioctl, void __user *argp)
> > {
> > struct file *eventfp, *filep = NULL;
> > @@ -806,6 +855,12 @@ long vhost_vring_ioctl(struct vhost_dev *d, int ioctl, void __user *argp)
> > } else
> > filep = eventfp;
> > break;
> > + case VHOST_SET_VRING_ENDIAN_LEGACY:
> > + r = vhost_set_vring_endian_legacy(vq, argp);
> > + break;
> > + case VHOST_GET_VRING_ENDIAN_LEGACY:
> > + r = vhost_get_vring_endian_legacy(vq, idx, argp);
> > + break;
> > default:
> > r = -ENOIOCTLCMD;
> > }
> > diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> > index 4e9a186..981ba06 100644
> > --- a/drivers/vhost/vhost.h
> > +++ b/drivers/vhost/vhost.h
> > @@ -106,6 +106,9 @@ struct vhost_virtqueue {
> > /* Log write descriptors */
> > void __user *log_base;
> > struct vhost_log *log;
> > +
> > + /* We need to know the device endianness with legacy virtio. */
> > + bool legacy_is_little_endian;
> > };
> >
> > struct vhost_dev {
> > @@ -173,11 +176,23 @@ static inline bool vhost_has_feature(struct vhost_virtqueue *vq, int bit)
> > return vq->acked_features & (1ULL << bit);
> > }
> >
> > +#ifdef CONFIG_VHOST_SET_ENDIAN_LEGACY
> > +static inline bool vhost_legacy_is_little_endian(struct vhost_virtqueue *vq)
> > +{
> > + return vq->legacy_is_little_endian;
> > +}
> > +#else
> > +static inline bool vhost_legacy_is_little_endian(struct vhost_virtqueue *vq)
> > +{
> > + return virtio_legacy_is_little_endian();
> > +}
> > +#endif
> > +
> > static inline bool vhost_is_little_endian(struct vhost_virtqueue *vq)
> > {
> > if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
> > return true;
> > - return virtio_legacy_is_little_endian();
> > + return vhost_legacy_is_little_endian(vq);
> > }
> >
> > /* Memory accessors */
> > diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> > index bb6a5b4..1b01a72 100644
> > --- a/include/uapi/linux/vhost.h
> > +++ b/include/uapi/linux/vhost.h
> > @@ -103,6 +103,11 @@ struct vhost_memory {
> > /* Get accessor: reads index, writes value in num */
> > #define VHOST_GET_VRING_BASE _IOWR(VHOST_VIRTIO, 0x12, struct vhost_vring_state)
> >
> > +/* Set endianness for the ring (legacy virtio only) */
> > +/* num is 0 for big endian, other values mean little endian */
> > +#define VHOST_SET_VRING_ENDIAN_LEGACY _IOW(VHOST_VIRTIO, 0x13, struct vhost_vring_state)
> > +#define VHOST_GET_VRING_ENDIAN_LEGACY _IOW(VHOST_VIRTIO, 0x14, struct vhost_vring_state)
> > +
> > /* The following ioctls use eventfd file descriptors to signal and poll
> > * for events. */
> >
>
> So if the config feature is enabled, you actually check two
> things: 1. feature 2. ioctl
>
> Why not override is_le when we set VIRTIO_F_VERSION_1?
> I guess you are worried that setting a value and not being
> able to read it back is ugly. We can add a flag to track
> it though. So here's an idea
> I would probably rename to G/SET_BIG_ENDIAN then it's obvious
> it's legacy. And just document that it's ignored with
> VIRTIO_F_VERSION_1.
>
> make sure it's not changed when ring is running
>
> simply set vq->is_le correctly when we start/stop the device
> vq->is_le = vhost_has_feature(vq, VIRTIO_F_VERSION_1) ||
> !vq->user_be;
>
> static inline bool vhost_is_little_endian(struct vhost_virtqueue *vq)
> {
> return vq->is_le;
> }
>
>
I like the idea. I'll give it a try.
Thanks for your help Michael.
--
Greg
^ permalink raw reply
* Re: [PATCH v6 00/15] Add support to STMicroelectronics STM32 family
From: Maxime Coquelin @ 2015-04-08 9:14 UTC (permalink / raw)
To: Arnd Bergmann, olof-nZhT3qVonbNeoWH0uzbU5w
Cc: Jonathan Corbet, Pawel Moll, Mark Rutland, Ian Campbell,
Kumar Gala, Daniel Lezcano, Uwe Kleine-König,
Andreas Färber, Geert Uytterhoeven, Rob Herring,
Philipp Zabel, Linus Walleij, Stefan Agner, Peter Meerwald,
Paul Bolle, Peter Hurley, Andy Shevchenko, Chanwoo Choi,
Russell King, Thomas Gleixner, Joe Perches, Greg Kroah-Hartman
In-Reply-To: <1428424234-28572-1-git-send-email-mcoquelin.stm32-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Hi Arnd, Olof,
2015-04-07 18:30 GMT+02:00 Maxime Coquelin <mcoquelin.stm32-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> This sixth round adds minor fixes to USART driver, and adds some Acks.
>
> STM32 MCUs are Cortex-M CPU, used in various applications (consumer
> electronics, industrial applications, hobbyists...).
> Datasheets, user and programming manuals are publicly available on
> STMicroelectronics website.
>
> With this series applied, the STM32F419 Discovery can boot succesfully.
>
> Changes since v5:
> -----------------
> - Change st,hw-flow-ctrl property to auto-flow-control (Rob)
> - Constify stm32_uart_ops (Joe)
> - Propagate request_irq error in USART driver (Andy)
> - Applies Acked-by and Reviewed-by (Rob, Peter)
>
> Changes since v4:
> -----------------
> - Cosmetic changes in USART driver (Andy)
> - Apply Acks on reset driver & bindings (Philipp & Rob)
>
> Changes since v3:
> -----------------
> - Fix and simplify error path in ARMv7-M Systick driver (Daniel)
> - Improve reset bindings documentation (Philipp)
> - Fix trailing lines anf typos in reset driver & doc (Philipp & Chanwoo)
> - Fix MODULE_LICENCE in USART driver (Paul)
> - Refactor USART baudrate calculation (Peter & Andy)
> - Fix error path in USART init (Peter & Russell)
> - Fix HW flow control in USART driver (Peter)
> - Fix serial port type number to unused one (Peter)
> - Applies Chanwoo's Tested-by on the series
>
> Changes since v2:
> -----------------
> - Remove pinctrl driver from the series.
> - Remove reset_controller_of_init(), and reset the timers in the bootloader
> - Add HW flow contrl property for serial driver
> - Lots of changes in the DTS file, as per Andreas recommendations
> - Some Kconfig clean-ups
> - Adapt the config to be compatible with Andreas' bootwrapper, except UART port.
> - Various fixes in documentation
>
> Changes since v1:
> -----------------
> - Move bindings documentation in their own patches (Andreas)
> - Rename ARM System timer to armv7m-systick (Rob)
> - Add clock-frequency property handling in armv7m-systick (Rob)
> - Re-factor the reset controllers into a single controller (Philipp)
> - Add kerneldoc to reset_controller_of_init (Philipp)
> - Add named constants in include/dt-bindings/reset/ (Philipp)
> - Make pinctrl driver to depend on ARCH_STM32 or COMPILE_TEST (Geert)
> - Introduce CPUV7M_NUM_IRQ config flag to indicate the number of interrupts
> supported by the MCU, in order to limit memory waste in vectors' table (Uwe)
>
> Maxime Coquelin (15):
> scripts: link-vmlinux: Don't pass page offset to kallsyms if XIP
> Kernel
> ARM: ARMv7-M: Enlarge vector table up to 256 entries
> dt-bindings: Document the ARM System timer bindings
> clocksource/drivers: Add ARM System timer driver
> dt-bindings: Document the STM32 reset bindings
> drivers: reset: Add STM32 reset driver
> dt-bindings: Document the STM32 timer bindings
> clockevents/drivers: Add STM32 Timer driver
> dt-bindings: Document the STM32 USART bindings
> serial: stm32-usart: Add STM32 USART Driver
> ARM: Add STM32 family machine
> ARM: dts: Add ARM System timer as clockevent in armv7m
> ARM: dts: Introduce STM32F429 MCU
> ARM: configs: Add STM32 defconfig
> MAINTAINERS: Add entry for STM32 MCUs
>
> Documentation/arm/stm32/overview.txt | 32 +
> Documentation/arm/stm32/stm32f429-overview.txt | 22 +
> .../devicetree/bindings/arm/armv7m_systick.txt | 26 +
> .../devicetree/bindings/reset/st,stm32-rcc.txt | 107 +++
> .../devicetree/bindings/serial/st,stm32-usart.txt | 32 +
> .../devicetree/bindings/timer/st,stm32-timer.txt | 22 +
> MAINTAINERS | 8 +
> arch/arm/Kconfig | 18 +
> arch/arm/Makefile | 1 +
> arch/arm/boot/dts/Makefile | 1 +
> arch/arm/boot/dts/armv7-m.dtsi | 6 +
> arch/arm/boot/dts/stm32f429-disco.dts | 71 ++
> arch/arm/boot/dts/stm32f429.dtsi | 226 +++++++
> arch/arm/configs/stm32_defconfig | 71 ++
> arch/arm/kernel/entry-v7m.S | 13 +-
> arch/arm/mach-stm32/Makefile | 1 +
> arch/arm/mach-stm32/Makefile.boot | 3 +
> arch/arm/mach-stm32/board-dt.c | 19 +
> arch/arm/mm/Kconfig | 15 +
> drivers/clocksource/Kconfig | 15 +
> drivers/clocksource/Makefile | 2 +
> drivers/clocksource/armv7m_systick.c | 79 +++
> drivers/clocksource/timer-stm32.c | 184 ++++++
> drivers/reset/Makefile | 1 +
> drivers/reset/reset-stm32.c | 124 ++++
> drivers/tty/serial/Kconfig | 17 +
> drivers/tty/serial/Makefile | 1 +
> drivers/tty/serial/stm32-usart.c | 735 +++++++++++++++++++++
> include/uapi/linux/serial_core.h | 3 +
> scripts/link-vmlinux.sh | 2 +-
> 30 files changed, 1852 insertions(+), 5 deletions(-)
> create mode 100644 Documentation/arm/stm32/overview.txt
> create mode 100644 Documentation/arm/stm32/stm32f429-overview.txt
> create mode 100644 Documentation/devicetree/bindings/arm/armv7m_systick.txt
> create mode 100644 Documentation/devicetree/bindings/reset/st,stm32-rcc.txt
> create mode 100644 Documentation/devicetree/bindings/serial/st,stm32-usart.txt
> create mode 100644 Documentation/devicetree/bindings/timer/st,stm32-timer.txt
> create mode 100644 arch/arm/boot/dts/stm32f429-disco.dts
> create mode 100644 arch/arm/boot/dts/stm32f429.dtsi
> create mode 100644 arch/arm/configs/stm32_defconfig
> create mode 100644 arch/arm/mach-stm32/Makefile
> create mode 100644 arch/arm/mach-stm32/Makefile.boot
> create mode 100644 arch/arm/mach-stm32/board-dt.c
> create mode 100644 drivers/clocksource/armv7m_systick.c
> create mode 100644 drivers/clocksource/timer-stm32.c
> create mode 100644 drivers/reset/reset-stm32.c
> create mode 100644 drivers/tty/serial/stm32-usart.c
>
> --
> 1.9.1
>
Do you think we could consider this series for v4.1?
I can prepare a pull request if needed.
Maybe it is too late, but since it is mainly new files, the risk of
conflicts is rather low.
Kind regards,
Maxime
^ permalink raw reply
* Re: [PATCH v4 05/10] eeprom: Add bindings for simple eeprom framework
From: Srinivas Kandagatla @ 2015-04-08 9:24 UTC (permalink / raw)
To: Matt Porter
Cc: Mark Brown, Rob Herring, linux-arm-kernel@lists.infradead.org,
devicetree@vger.kernel.org, Arnd Bergmann, Greg Kroah-Hartman,
Sascha Hauer, Stephen Boyd, linux-kernel@vger.kernel.org,
Rob Herring, Kumar Gala, Maxime Ripard, linux-api@vger.kernel.org,
linux-arm-msm
In-Reply-To: <20150407194636.GB6662@beef>
On 07/04/15 20:46, Matt Porter wrote:
> On Tue, Apr 07, 2015 at 07:03:30PM +0100, Srinivas Kandagatla wrote:
>>
>>
>> On 07/04/15 18:46, Mark Brown wrote:
>>> On Tue, Apr 07, 2015 at 06:35:49PM +0100, Srinivas Kandagatla wrote:
>>>> On 06/04/15 16:04, Matt Porter wrote:
>>>>> On Mon, Apr 06, 2015 at 09:11:05AM -0500, Rob Herring wrote:
>>>
>>>>>>> The generic binding could really use a "read-only" property here as this
>>>>>>> is a common hardware attribute for many nvmem devices. A serial EEPROM
>>>
>>>> Correct me If am wrong.
>>>
>>>> Regarding write protection/read-only, regmap already has provisions to
>>>> support this feature. regmap would bail out with errors if any attempt to
>>>> write to non-writable regions. It all depends on the data providers how they
>>>> setup the regmap and the bindings for those are specific individual data
>>>> providers I think.
>>>
>>> There is the ability to flag read/write permissions in regmap but I
>>> think there's some suggestion that this should be exposed to userspace
>>> so that it's easier for it to handle things rather than just writing
>>> then coping with any errors.
>>
>> Yes, That's possible if the data provider use the "read-only" generic
>> binding like MTD partitions which the eeprom framwork could use to set the
>> binary file mode appropriately.
>
> Right, that's what Rob suggested as to how it should be exposed to
> userspace. I think Mark is suggesting that it can also be done by
> returning appropriately fine-grained error codes from a writeable
> attribute.
>
We are taking about two things here.
1: "Setting the correct mode for the user space binary file."
This is only possible if the entire eeprom/nvmem has write protection.
We could get that info from 2 places. One having a new DT property
bindings like "read-only" as Rob suggested.
or
Second, scan the regmap for writeable attributes and the correct file
mode. For this option AFAIK, with existing regmap code we can only do
this by scanning the entire range, which is bad I guess.
2: "Returning appropriate error codes to user space."
This is already addressed in the existing code, regmap would return an
(-EIO) I/O error Or error codes from providers in cases where an write
attempt to non writable register/or something wrong is made and the
*same error* code is passed back to user too. All the error codes from
regmap/provider layer are always passed back to the user space. These
error code, I supposed are fine grained originating from the low layer.
I think with "read-only" property and returning same error codes from
regmap/provider layer to user-space would address the points raised by Matt.
> Just to clarify here, I brought this up because if we only allow
> the writes to fail, there's not necessarily not enough information
> available to know if they failed due to a real error (perhaps write
> cycles for the underlying nvmem device have been exhausted) or is
> it simply that the underlying device has been hardware write protected
> (such as as the write protect pin hardwired that way or it's an OTP
> device). The client, whether userspace or otherwise is going to need
> to know this information to make informed policy decisions.
>
Thanks for the inputs.
The exiting regmap writeable_register api will only return true or
false. But if the provider implements its own read/writes functions, the
error-codes from these apis would be passed back to user anyway, so the
user can make informed policy decisions. This logic already present in
with the exiting eeprom/regmap code.
--srini
> -Matt
>
>> "read-only" property seems to be more generic for all types of data
>> providers.
>>
>> I will give it a try and document this in the bindings too in next version.
>>
>> --srini
>>>
^ permalink raw reply
* [PATCH 0/5] Ensure that lun ids are contiguous
From: Krzysztof Opasiak @ 2015-04-08 11:28 UTC (permalink / raw)
To: balbi-l0cyMroinI0, gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
jlbec-aKy9MeLSZ9dg9hUCZPvPmw
Cc: andrzej.p-Sze3O3UU22JBDgjK7y7TUQ,
m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA, Krzysztof Opasiak
Dear list,
This series fix configfs interface for mass storage function.
According to mass storage specification[1]:
"Logical Unit Numbers on the device shall be numbered contiguously
starting from LUN 0 to a maximum LUN of 15 (Fh)."
Currently configfs interface allows to create LUNs with
arbitrary ids what causes problems with some host side
mass storage drivers.
This patch extends configfs interface with unlocked version
of configfs_depend_item() which should be used only in configfs
callbacks. This allows to protect from
removing lun directory from the middle of id space.
Example:
as is:
$ mkdir mass_storage.name
$ mkdir lun.3
$ mkdir lun.5
$ rmdir lun.3
After this series:
$ mkdir mass_storage.name
$ mkdir lun.3
mkdir: cannot create directory 'lun.3': Invalid argument
$ mkdir lun.1
$ mkdir lun.2
$ rmdir lun.1
rmdir: failed to remove 'lun.1': Device or resource busy
$ rmdir lun.2
$ rmdir lun.1
--
Best regards,
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics
Krzysztof Opasiak (5):
fs: configfs: Fix typo in comment
fs: configfs: Add unlocked version of configfs_depend_item()
usb: gadget: mass_storage: Store lun_opts in fsg_opts
usb: gadget: mass_storage: Ensure that lun ids are contiguous
Documentation: ABI: Fix documentation for mass_storage function
.../ABI/testing/configfs-usb-gadget-mass-storage | 7 +++-
drivers/usb/gadget/function/f_mass_storage.c | 40 ++++++++++++++++++--
drivers/usb/gadget/function/f_mass_storage.h | 1 +
fs/configfs/dir.c | 31 ++++++++++++++-
include/linux/configfs.h | 9 +++++
5 files changed, 82 insertions(+), 6 deletions(-)
--
1.7.9.5
^ permalink raw reply
* [PATCH 1/5] fs: configfs: Fix typo in comment
From: Krzysztof Opasiak @ 2015-04-08 11:28 UTC (permalink / raw)
To: balbi, gregkh, jlbec
Cc: andrzej.p, m.szyprowski, linux-api, linux-kernel, linux-usb,
Krzysztof Opasiak
In-Reply-To: <1428492528-11993-1-git-send-email-k.opasiak@samsung.com>
Signed-off-by: Krzysztof Opasiak <k.opasiak@samsung.com>
---
fs/configfs/dir.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index cf0db00..dee1cb5 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -325,7 +325,7 @@ static void configfs_dir_set_ready(struct configfs_dirent *sd)
* attached and not validated yet.
* @sd configfs_dirent of the directory to check
*
- * @return non-zero iff the directory was validated
+ * @return non-zero if the directory was validated
*
* Note: takes configfs_dirent_lock, so the result may change from false to true
* in two consecutive calls, but never from true to false.
--
1.7.9.5
^ permalink raw reply related
* [PATCH 2/5] fs: configfs: Add unlocked version of configfs_depend_item()
From: Krzysztof Opasiak @ 2015-04-08 11:28 UTC (permalink / raw)
To: balbi, gregkh, jlbec
Cc: andrzej.p, m.szyprowski, linux-api, linux-kernel, linux-usb,
Krzysztof Opasiak
In-Reply-To: <1428492528-11993-1-git-send-email-k.opasiak@samsung.com>
Sometimes it might be desirable to prohibit removing a directory
in configfs. One example is USB gadget (mass_storage function):
when gadget is already bound, if lun directory is removed,
the gadget must be thrown away, too. A better solution would be
to fail with e.g. -EBUSY.
Currently configfs has configfs_depend/undepend_item() methods
but they cannot be used in configfs callbacks. This commit
adds unlocked version of this methods which can be used
only in configfs callbacks.
Signed-off-by: Krzysztof Opasiak <k.opasiak@samsung.com>
---
fs/configfs/dir.c | 29 +++++++++++++++++++++++++++++
include/linux/configfs.h | 9 +++++++++
2 files changed, 38 insertions(+)
diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index dee1cb5..ef51594 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -1152,6 +1152,35 @@ void configfs_undepend_item(struct configfs_subsystem *subsys,
}
EXPORT_SYMBOL(configfs_undepend_item);
+int configfs_depend_item_unlocked(struct config_item *target)
+{
+ struct configfs_dirent *sd;
+ int ret = -ENOENT;
+
+ spin_lock(&configfs_dirent_lock);
+ BUG_ON(!target->ci_dentry);
+
+ sd = target->ci_dentry->d_fsdata;
+ if ((sd->s_type & CONFIGFS_DIR) &&
+ ((sd->s_type & CONFIGFS_USET_DROPPING) ||
+ (sd->s_type & CONFIGFS_USET_CREATING)))
+ goto out_unlock_dirent_lock;
+
+ sd->s_dependent_count += 1;
+ ret = 0;
+
+out_unlock_dirent_lock:
+ spin_unlock(&configfs_dirent_lock);
+ return ret;
+}
+EXPORT_SYMBOL(configfs_depend_item_unlocked);
+
+void configfs_undepend_item_unlocked(struct config_item *target)
+{
+ configfs_undepend_item(NULL, target);
+}
+EXPORT_SYMBOL(configfs_undepend_item_unlocked);
+
static int configfs_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode)
{
int ret = 0;
diff --git a/include/linux/configfs.h b/include/linux/configfs.h
index 34025df..e9dbf01 100644
--- a/include/linux/configfs.h
+++ b/include/linux/configfs.h
@@ -257,4 +257,13 @@ void configfs_unregister_subsystem(struct configfs_subsystem *subsys);
int configfs_depend_item(struct configfs_subsystem *subsys, struct config_item *target);
void configfs_undepend_item(struct configfs_subsystem *subsys, struct config_item *target);
+/*
+ * These functions can sleep and can alloc with GFP_KERNEL
+ * NOTE: These should be called only underneath configfs callbacks.
+ * WARNING: These cannot be called on newly created item
+ * (in make_group()/make_item callback)
+ */
+int configfs_depend_item_unlocked(struct config_item *target);
+void configfs_undepend_item_unlocked(struct config_item *target);
+
#endif /* _CONFIGFS_H_ */
--
1.7.9.5
^ permalink raw reply related
* [PATCH 3/5] usb: gadget: mass_storage: Store lun_opts in fsg_opts
From: Krzysztof Opasiak @ 2015-04-08 11:28 UTC (permalink / raw)
To: balbi, gregkh, jlbec
Cc: andrzej.p, m.szyprowski, linux-api, linux-kernel, linux-usb,
Krzysztof Opasiak
In-Reply-To: <1428492528-11993-1-git-send-email-k.opasiak@samsung.com>
Signed-off-by: Krzysztof Opasiak <k.opasiak@samsung.com>
---
drivers/usb/gadget/function/f_mass_storage.c | 5 +++++
drivers/usb/gadget/function/f_mass_storage.h | 1 +
2 files changed, 6 insertions(+)
diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
index 811929c..095b618 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -3372,6 +3372,8 @@ static struct config_group *fsg_lun_make(struct config_group *group,
}
opts->lun = fsg_opts->common->luns[num];
opts->lun_id = num;
+ BUG_ON(fsg_opts->lun_opts[num]);
+ fsg_opts->lun_opts[num] = opts;
mutex_unlock(&fsg_opts->lock);
config_group_init_type_name(&opts->group, name, &fsg_lun_type);
@@ -3400,6 +3402,7 @@ static void fsg_lun_drop(struct config_group *group, struct config_item *item)
fsg_common_remove_lun(lun_opts->lun, fsg_opts->common->sysfs);
fsg_opts->common->luns[lun_opts->lun_id] = NULL;
+ fsg_opts->lun_opts[lun_opts->lun_id] = NULL;
lun_opts->lun_id = 0;
mutex_unlock(&fsg_opts->lock);
@@ -3546,6 +3549,7 @@ static struct usb_function_instance *fsg_alloc_inst(void)
if (!opts)
return ERR_PTR(-ENOMEM);
mutex_init(&opts->lock);
+ memset(opts->lun_opts, 0, sizeof(opts->lun_opts));
opts->func_inst.free_func_inst = fsg_free_inst;
opts->common = fsg_common_setup(opts->common);
if (IS_ERR(opts->common)) {
@@ -3569,6 +3573,7 @@ static struct usb_function_instance *fsg_alloc_inst(void)
(const char **)&opts->func_inst.group.cg_item.ci_name);
opts->lun0.lun = opts->common->luns[0];
opts->lun0.lun_id = 0;
+ opts->lun_opts[0] = &opts->lun0;
config_group_init_type_name(&opts->lun0.group, "lun.0", &fsg_lun_type);
opts->default_groups[0] = &opts->lun0.group;
opts->func_inst.group.default_groups = opts->default_groups;
diff --git a/drivers/usb/gadget/function/f_mass_storage.h b/drivers/usb/gadget/function/f_mass_storage.h
index b4866fc..0a7c656 100644
--- a/drivers/usb/gadget/function/f_mass_storage.h
+++ b/drivers/usb/gadget/function/f_mass_storage.h
@@ -81,6 +81,7 @@ struct fsg_opts {
struct fsg_common *common;
struct usb_function_instance func_inst;
struct fsg_lun_opts lun0;
+ struct fsg_lun_opts *lun_opts[FSG_MAX_LUNS];
struct config_group *default_groups[2];
bool no_configfs; /* for legacy gadgets */
--
1.7.9.5
^ permalink raw reply related
* [PATCH 4/5] usb: gadget: mass_storage: Ensure that lun ids are contiguous
From: Krzysztof Opasiak @ 2015-04-08 11:28 UTC (permalink / raw)
To: balbi-l0cyMroinI0, gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
jlbec-aKy9MeLSZ9dg9hUCZPvPmw
Cc: andrzej.p-Sze3O3UU22JBDgjK7y7TUQ,
m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA, Krzysztof Opasiak
In-Reply-To: <1428492528-11993-1-git-send-email-k.opasiak-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Signed-off-by: Krzysztof Opasiak <k.opasiak-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
drivers/usb/gadget/function/f_mass_storage.c | 35 +++++++++++++++++++++++---
1 file changed, 31 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
index 095b618..9d9fafb 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -3355,6 +3355,12 @@ static struct config_group *fsg_lun_make(struct config_group *group,
goto out;
}
+ if (!fsg_opts->common->luns[num - 1]) {
+ ret = -EINVAL;
+ pr_err("LUN ids should be contignous\n");
+ goto out;
+ }
+
opts = kzalloc(sizeof(*opts), GFP_KERNEL);
if (!opts) {
ret = -ENOMEM;
@@ -3364,12 +3370,17 @@ static struct config_group *fsg_lun_make(struct config_group *group,
memset(&config, 0, sizeof(config));
config.removable = true;
+ /* ensure that lun ids are contiguous */
+ ret = configfs_depend_item_unlocked(&(fsg_opts->lun_opts
+ [num - 1]->group.cg_item));
+ if (ret)
+ goto err_free_opts;
+
ret = fsg_common_create_lun(fsg_opts->common, &config, num, name,
(const char **)&group->cg_item.ci_name);
- if (ret) {
- kfree(opts);
- goto out;
- }
+ if (ret)
+ goto err_undepend_item;
+
opts->lun = fsg_opts->common->luns[num];
opts->lun_id = num;
BUG_ON(fsg_opts->lun_opts[num]);
@@ -3382,6 +3393,16 @@ static struct config_group *fsg_lun_make(struct config_group *group,
out:
mutex_unlock(&fsg_opts->lock);
return ERR_PTR(ret);
+
+err_undepend_item:
+ configfs_undepend_item_unlocked(&(fsg_opts->lun_opts
+ [num - 1]->group.cg_item));
+err_free_opts:
+ kfree(opts);
+
+ mutex_unlock(&fsg_opts->lock);
+ return ERR_PTR(ret);
+
}
static void fsg_lun_drop(struct config_group *group, struct config_item *item)
@@ -3400,10 +3421,16 @@ static void fsg_lun_drop(struct config_group *group, struct config_item *item)
unregister_gadget_item(gadget);
}
+
+ /* Allow to remove next one */
+ configfs_undepend_item_unlocked(&(fsg_opts->lun_opts
+ [lun_opts->lun_id - 1]->group.cg_item));
+
fsg_common_remove_lun(lun_opts->lun, fsg_opts->common->sysfs);
fsg_opts->common->luns[lun_opts->lun_id] = NULL;
fsg_opts->lun_opts[lun_opts->lun_id] = NULL;
lun_opts->lun_id = 0;
+
mutex_unlock(&fsg_opts->lock);
config_item_put(item);
--
1.7.9.5
^ permalink raw reply related
* [PATCH 5/5] Documentation: ABI: Fix documentation for mass_storage function
From: Krzysztof Opasiak @ 2015-04-08 11:28 UTC (permalink / raw)
To: balbi-l0cyMroinI0, gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
jlbec-aKy9MeLSZ9dg9hUCZPvPmw
Cc: andrzej.p-Sze3O3UU22JBDgjK7y7TUQ,
m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA, Krzysztof Opasiak
In-Reply-To: <1428492528-11993-1-git-send-email-k.opasiak-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Luns in mass storage function are identified using their id.
While creating lun's directory user cannot choose any arbitrary
name other than arabic numeral from 1 to FSG_MAX_LUNS.
Signed-off-by: Krzysztof Opasiak <k.opasiak-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
.../ABI/testing/configfs-usb-gadget-mass-storage | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/Documentation/ABI/testing/configfs-usb-gadget-mass-storage b/Documentation/ABI/testing/configfs-usb-gadget-mass-storage
index 9931fb0..0b54280 100644
--- a/Documentation/ABI/testing/configfs-usb-gadget-mass-storage
+++ b/Documentation/ABI/testing/configfs-usb-gadget-mass-storage
@@ -11,10 +11,15 @@ Description:
are 2..4. Available only if
CONFIG_USB_GADGET_DEBUG_FILES is set.
-What: /config/usb-gadget/gadget/functions/mass_storage.name/lun.name
+What: /config/usb-gadget/gadget/functions/mass_storage.name/lun.id
Date: Oct 2013
KernelVersion: 3.13
Description:
+ id - arabic numeral from 1 to FSG_MAX_LUNS
+ (which is 8 by default) - 1. LUNs should be numbered contiguously.
+ lun.0 is reserved for default lun which appears while creating
+ mass_storage.name directory and cannot be removed by the user.
+
The attributes:
file - The path to the backing file for the LUN.
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH v2 0/4] Ensure that lun ids are contiguous
From: Krzysztof Opasiak @ 2015-04-08 12:06 UTC (permalink / raw)
To: balbi, gregkh, jlbec
Cc: andrzej.p, m.szyprowski, linux-api, linux-kernel, linux-usb,
Krzysztof Opasiak
Dear list,
This series fix configfs interface for mass storage function.
According to mass storage specification[1]:
"Logical Unit Numbers on the device shall be numbered contiguously
starting from LUN 0 to a maximum LUN of 15 (Fh)."
Currently configfs interface allows to create LUNs with
arbitrary ids what causes problems with some host side
mass storage drivers.
This patch extends configfs interface with unlocked version
of configfs_depend_item() which should be used only in configfs
callbacks. This allows to protect from
removing lun directory from the middle of id space.
Example:
as is:
$ mkdir mass_storage.name
$ mkdir lun.3
$ mkdir lun.5
$ rmdir lun.3
After this series:
$ mkdir mass_storage.name
$ mkdir lun.3
mkdir: cannot create directory 'lun.3': Invalid argument
$ mkdir lun.1
$ mkdir lun.2
$ rmdir lun.1
rmdir: failed to remove 'lun.1': Device or resource busy
$ rmdir lun.2
$ rmdir lun.1
--
Best regards,
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics
---
Changes since v1:
- drop incorrect typo fix
("iff" is not a typo but shorten version of "if and only if")
Krzysztof Opasiak (4):
fs: configfs: Add unlocked version of configfs_depend_item()
usb: gadget: mass_storage: Store lun_opts in fsg_opts
usb: gadget: mass_storage: Ensure that lun ids are contiguous
Documentation: ABI: Fix documentation for mass_storage function
.../ABI/testing/configfs-usb-gadget-mass-storage | 7 +++-
drivers/usb/gadget/function/f_mass_storage.c | 40 ++++++++++++++++++--
drivers/usb/gadget/function/f_mass_storage.h | 1 +
fs/configfs/dir.c | 29 ++++++++++++++
include/linux/configfs.h | 9 +++++
5 files changed, 81 insertions(+), 5 deletions(-)
--
1.7.9.5
^ permalink raw reply
* [PATCH v2 1/4] fs: configfs: Add unlocked version of configfs_depend_item()
From: Krzysztof Opasiak @ 2015-04-08 12:06 UTC (permalink / raw)
To: balbi, gregkh, jlbec
Cc: andrzej.p, m.szyprowski, linux-api, linux-kernel, linux-usb,
Krzysztof Opasiak
In-Reply-To: <1428494808-12566-1-git-send-email-k.opasiak@samsung.com>
Sometimes it might be desirable to prohibit removing a directory
in configfs. One example is USB gadget (mass_storage function):
when gadget is already bound, if lun directory is removed,
the gadget must be thrown away, too. A better solution would be
to fail with e.g. -EBUSY.
Currently configfs has configfs_depend/undepend_item() methods
but they cannot be used in configfs callbacks. This commit
adds unlocked version of this methods which can be used
only in configfs callbacks.
Signed-off-by: Krzysztof Opasiak <k.opasiak@samsung.com>
---
fs/configfs/dir.c | 29 +++++++++++++++++++++++++++++
include/linux/configfs.h | 9 +++++++++
2 files changed, 38 insertions(+)
diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index cf0db00..7875a5e 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -1152,6 +1152,35 @@ void configfs_undepend_item(struct configfs_subsystem *subsys,
}
EXPORT_SYMBOL(configfs_undepend_item);
+int configfs_depend_item_unlocked(struct config_item *target)
+{
+ struct configfs_dirent *sd;
+ int ret = -ENOENT;
+
+ spin_lock(&configfs_dirent_lock);
+ BUG_ON(!target->ci_dentry);
+
+ sd = target->ci_dentry->d_fsdata;
+ if ((sd->s_type & CONFIGFS_DIR) &&
+ ((sd->s_type & CONFIGFS_USET_DROPPING) ||
+ (sd->s_type & CONFIGFS_USET_CREATING)))
+ goto out_unlock_dirent_lock;
+
+ sd->s_dependent_count += 1;
+ ret = 0;
+
+out_unlock_dirent_lock:
+ spin_unlock(&configfs_dirent_lock);
+ return ret;
+}
+EXPORT_SYMBOL(configfs_depend_item_unlocked);
+
+void configfs_undepend_item_unlocked(struct config_item *target)
+{
+ configfs_undepend_item(NULL, target);
+}
+EXPORT_SYMBOL(configfs_undepend_item_unlocked);
+
static int configfs_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode)
{
int ret = 0;
diff --git a/include/linux/configfs.h b/include/linux/configfs.h
index 34025df..e9dbf01 100644
--- a/include/linux/configfs.h
+++ b/include/linux/configfs.h
@@ -257,4 +257,13 @@ void configfs_unregister_subsystem(struct configfs_subsystem *subsys);
int configfs_depend_item(struct configfs_subsystem *subsys, struct config_item *target);
void configfs_undepend_item(struct configfs_subsystem *subsys, struct config_item *target);
+/*
+ * These functions can sleep and can alloc with GFP_KERNEL
+ * NOTE: These should be called only underneath configfs callbacks.
+ * WARNING: These cannot be called on newly created item
+ * (in make_group()/make_item callback)
+ */
+int configfs_depend_item_unlocked(struct config_item *target);
+void configfs_undepend_item_unlocked(struct config_item *target);
+
#endif /* _CONFIGFS_H_ */
--
1.7.9.5
^ permalink raw reply related
* [PATCH v2 2/4] usb: gadget: mass_storage: Store lun_opts in fsg_opts
From: Krzysztof Opasiak @ 2015-04-08 12:06 UTC (permalink / raw)
To: balbi, gregkh, jlbec
Cc: andrzej.p, m.szyprowski, linux-api, linux-kernel, linux-usb,
Krzysztof Opasiak
In-Reply-To: <1428494808-12566-1-git-send-email-k.opasiak@samsung.com>
Signed-off-by: Krzysztof Opasiak <k.opasiak@samsung.com>
---
drivers/usb/gadget/function/f_mass_storage.c | 5 +++++
drivers/usb/gadget/function/f_mass_storage.h | 1 +
2 files changed, 6 insertions(+)
diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
index 811929c..095b618 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -3372,6 +3372,8 @@ static struct config_group *fsg_lun_make(struct config_group *group,
}
opts->lun = fsg_opts->common->luns[num];
opts->lun_id = num;
+ BUG_ON(fsg_opts->lun_opts[num]);
+ fsg_opts->lun_opts[num] = opts;
mutex_unlock(&fsg_opts->lock);
config_group_init_type_name(&opts->group, name, &fsg_lun_type);
@@ -3400,6 +3402,7 @@ static void fsg_lun_drop(struct config_group *group, struct config_item *item)
fsg_common_remove_lun(lun_opts->lun, fsg_opts->common->sysfs);
fsg_opts->common->luns[lun_opts->lun_id] = NULL;
+ fsg_opts->lun_opts[lun_opts->lun_id] = NULL;
lun_opts->lun_id = 0;
mutex_unlock(&fsg_opts->lock);
@@ -3546,6 +3549,7 @@ static struct usb_function_instance *fsg_alloc_inst(void)
if (!opts)
return ERR_PTR(-ENOMEM);
mutex_init(&opts->lock);
+ memset(opts->lun_opts, 0, sizeof(opts->lun_opts));
opts->func_inst.free_func_inst = fsg_free_inst;
opts->common = fsg_common_setup(opts->common);
if (IS_ERR(opts->common)) {
@@ -3569,6 +3573,7 @@ static struct usb_function_instance *fsg_alloc_inst(void)
(const char **)&opts->func_inst.group.cg_item.ci_name);
opts->lun0.lun = opts->common->luns[0];
opts->lun0.lun_id = 0;
+ opts->lun_opts[0] = &opts->lun0;
config_group_init_type_name(&opts->lun0.group, "lun.0", &fsg_lun_type);
opts->default_groups[0] = &opts->lun0.group;
opts->func_inst.group.default_groups = opts->default_groups;
diff --git a/drivers/usb/gadget/function/f_mass_storage.h b/drivers/usb/gadget/function/f_mass_storage.h
index b4866fc..0a7c656 100644
--- a/drivers/usb/gadget/function/f_mass_storage.h
+++ b/drivers/usb/gadget/function/f_mass_storage.h
@@ -81,6 +81,7 @@ struct fsg_opts {
struct fsg_common *common;
struct usb_function_instance func_inst;
struct fsg_lun_opts lun0;
+ struct fsg_lun_opts *lun_opts[FSG_MAX_LUNS];
struct config_group *default_groups[2];
bool no_configfs; /* for legacy gadgets */
--
1.7.9.5
^ permalink raw reply related
* [PATCH v2 3/4] usb: gadget: mass_storage: Ensure that lun ids are contiguous
From: Krzysztof Opasiak @ 2015-04-08 12:06 UTC (permalink / raw)
To: balbi, gregkh, jlbec
Cc: andrzej.p, m.szyprowski, linux-api, linux-kernel, linux-usb,
Krzysztof Opasiak
In-Reply-To: <1428494808-12566-1-git-send-email-k.opasiak@samsung.com>
Signed-off-by: Krzysztof Opasiak <k.opasiak@samsung.com>
---
drivers/usb/gadget/function/f_mass_storage.c | 35 +++++++++++++++++++++++---
1 file changed, 31 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
index 095b618..9d9fafb 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -3355,6 +3355,12 @@ static struct config_group *fsg_lun_make(struct config_group *group,
goto out;
}
+ if (!fsg_opts->common->luns[num - 1]) {
+ ret = -EINVAL;
+ pr_err("LUN ids should be contignous\n");
+ goto out;
+ }
+
opts = kzalloc(sizeof(*opts), GFP_KERNEL);
if (!opts) {
ret = -ENOMEM;
@@ -3364,12 +3370,17 @@ static struct config_group *fsg_lun_make(struct config_group *group,
memset(&config, 0, sizeof(config));
config.removable = true;
+ /* ensure that lun ids are contiguous */
+ ret = configfs_depend_item_unlocked(&(fsg_opts->lun_opts
+ [num - 1]->group.cg_item));
+ if (ret)
+ goto err_free_opts;
+
ret = fsg_common_create_lun(fsg_opts->common, &config, num, name,
(const char **)&group->cg_item.ci_name);
- if (ret) {
- kfree(opts);
- goto out;
- }
+ if (ret)
+ goto err_undepend_item;
+
opts->lun = fsg_opts->common->luns[num];
opts->lun_id = num;
BUG_ON(fsg_opts->lun_opts[num]);
@@ -3382,6 +3393,16 @@ static struct config_group *fsg_lun_make(struct config_group *group,
out:
mutex_unlock(&fsg_opts->lock);
return ERR_PTR(ret);
+
+err_undepend_item:
+ configfs_undepend_item_unlocked(&(fsg_opts->lun_opts
+ [num - 1]->group.cg_item));
+err_free_opts:
+ kfree(opts);
+
+ mutex_unlock(&fsg_opts->lock);
+ return ERR_PTR(ret);
+
}
static void fsg_lun_drop(struct config_group *group, struct config_item *item)
@@ -3400,10 +3421,16 @@ static void fsg_lun_drop(struct config_group *group, struct config_item *item)
unregister_gadget_item(gadget);
}
+
+ /* Allow to remove next one */
+ configfs_undepend_item_unlocked(&(fsg_opts->lun_opts
+ [lun_opts->lun_id - 1]->group.cg_item));
+
fsg_common_remove_lun(lun_opts->lun, fsg_opts->common->sysfs);
fsg_opts->common->luns[lun_opts->lun_id] = NULL;
fsg_opts->lun_opts[lun_opts->lun_id] = NULL;
lun_opts->lun_id = 0;
+
mutex_unlock(&fsg_opts->lock);
config_item_put(item);
--
1.7.9.5
^ permalink raw reply related
* [PATCH v2 4/4] Documentation: ABI: Fix documentation for mass_storage function
From: Krzysztof Opasiak @ 2015-04-08 12:06 UTC (permalink / raw)
To: balbi-l0cyMroinI0, gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
jlbec-aKy9MeLSZ9dg9hUCZPvPmw
Cc: andrzej.p-Sze3O3UU22JBDgjK7y7TUQ,
m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA, Krzysztof Opasiak
In-Reply-To: <1428494808-12566-1-git-send-email-k.opasiak-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Luns in mass storage function are identified using their id.
While creating lun's directory user cannot choose any arbitrary
name other than arabic numeral from 1 to FSG_MAX_LUNS.
Signed-off-by: Krzysztof Opasiak <k.opasiak-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
.../ABI/testing/configfs-usb-gadget-mass-storage | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/Documentation/ABI/testing/configfs-usb-gadget-mass-storage b/Documentation/ABI/testing/configfs-usb-gadget-mass-storage
index 9931fb0..0b54280 100644
--- a/Documentation/ABI/testing/configfs-usb-gadget-mass-storage
+++ b/Documentation/ABI/testing/configfs-usb-gadget-mass-storage
@@ -11,10 +11,15 @@ Description:
are 2..4. Available only if
CONFIG_USB_GADGET_DEBUG_FILES is set.
-What: /config/usb-gadget/gadget/functions/mass_storage.name/lun.name
+What: /config/usb-gadget/gadget/functions/mass_storage.name/lun.id
Date: Oct 2013
KernelVersion: 3.13
Description:
+ id - arabic numeral from 1 to FSG_MAX_LUNS
+ (which is 8 by default) - 1. LUNs should be numbered contiguously.
+ lun.0 is reserved for default lun which appears while creating
+ mass_storage.name directory and cannot be removed by the user.
+
The attributes:
file - The path to the backing file for the LUN.
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH v2 2/4] usb: gadget: mass_storage: Store lun_opts in fsg_opts
From: Alan Stern @ 2015-04-08 14:15 UTC (permalink / raw)
To: Krzysztof Opasiak
Cc: balbi-l0cyMroinI0, gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
jlbec-aKy9MeLSZ9dg9hUCZPvPmw, andrzej.p-Sze3O3UU22JBDgjK7y7TUQ,
m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1428494808-12566-3-git-send-email-k.opasiak-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
On Wed, 8 Apr 2015, Krzysztof Opasiak wrote:
> Signed-off-by: Krzysztof Opasiak <k.opasiak-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> ---
> drivers/usb/gadget/function/f_mass_storage.c | 5 +++++
> drivers/usb/gadget/function/f_mass_storage.h | 1 +
> 2 files changed, 6 insertions(+)
>
> diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
> index 811929c..095b618 100644
> --- a/drivers/usb/gadget/function/f_mass_storage.c
> +++ b/drivers/usb/gadget/function/f_mass_storage.c
> @@ -3372,6 +3372,8 @@ static struct config_group *fsg_lun_make(struct config_group *group,
> }
> opts->lun = fsg_opts->common->luns[num];
> opts->lun_id = num;
> + BUG_ON(fsg_opts->lun_opts[num]);
This is not a good idea. BUG_ON should hardly ever be used. In fact,
Linus has said that the only time BUG_ON should be used is when things
are so badly messed up that it is better to crash the computer than to
let it continue.
What's wrong with using WARN_ON instead?
> diff --git a/drivers/usb/gadget/function/f_mass_storage.h b/drivers/usb/gadget/function/f_mass_storage.h
> index b4866fc..0a7c656 100644
> --- a/drivers/usb/gadget/function/f_mass_storage.h
> +++ b/drivers/usb/gadget/function/f_mass_storage.h
> @@ -81,6 +81,7 @@ struct fsg_opts {
> struct fsg_common *common;
> struct usb_function_instance func_inst;
> struct fsg_lun_opts lun0;
> + struct fsg_lun_opts *lun_opts[FSG_MAX_LUNS];
This looks strange. Why is the entry for LUN 0 duplicated?
Alan Stern
^ permalink raw reply
* Re: [PATCH v2 2/4] usb: gadget: mass_storage: Store lun_opts in fsg_opts
From: Krzysztof Opasiak @ 2015-04-08 14:56 UTC (permalink / raw)
To: Alan Stern
Cc: balbi-l0cyMroinI0, gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
jlbec-aKy9MeLSZ9dg9hUCZPvPmw, andrzej.p-Sze3O3UU22JBDgjK7y7TUQ,
m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <Pine.LNX.4.44L0.1504081012240.1384-100000-IYeN2dnnYyZXsRXLowluHWD2FQJk+8+b@public.gmane.org>
Hi,
On 04/08/2015 04:15 PM, Alan Stern wrote:> On Wed, 8 Apr 2015, Krzysztof
Opasiak wrote:
>
>> Signed-off-by: Krzysztof Opasiak <k.opasiak-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>> ---
>> drivers/usb/gadget/function/f_mass_storage.c | 5 +++++
>> drivers/usb/gadget/function/f_mass_storage.h | 1 +
>> 2 files changed, 6 insertions(+)
>>
>> diff --git a/drivers/usb/gadget/function/f_mass_storage.c
b/drivers/usb/gadget/function/f_mass_storage.c
>> index 811929c..095b618 100644
>> --- a/drivers/usb/gadget/function/f_mass_storage.c
>> +++ b/drivers/usb/gadget/function/f_mass_storage.c
>> @@ -3372,6 +3372,8 @@ static struct config_group
*fsg_lun_make(struct config_group *group,
>> }
>> opts->lun = fsg_opts->common->luns[num];
>> opts->lun_id = num;
>> + BUG_ON(fsg_opts->lun_opts[num]);
>
> This is not a good idea. BUG_ON should hardly ever be used. In fact,
> Linus has said that the only time BUG_ON should be used is when things
> are so badly messed up that it is better to crash the computer than to
> let it continue.
>
> What's wrong with using WARN_ON instead?
Nothing. I have simply used BUG_ON() because this situation should never
happen. If it happed then we made some mess in luns and there is no easy
way to recovery from this point. This is only a little defense point for
future to make debugging easier. I may change this to WARN_ON() if you like.
>
>> diff --git a/drivers/usb/gadget/function/f_mass_storage.h
b/drivers/usb/gadget/function/f_mass_storage.h
>> index b4866fc..0a7c656 100644
>> --- a/drivers/usb/gadget/function/f_mass_storage.h
>> +++ b/drivers/usb/gadget/function/f_mass_storage.h
>> @@ -81,6 +81,7 @@ struct fsg_opts {
>> struct fsg_common *common;
>> struct usb_function_instance func_inst;
>> struct fsg_lun_opts lun0;
>> + struct fsg_lun_opts *lun_opts[FSG_MAX_LUNS];
>
> This looks strange. Why is the entry for LUN 0 duplicated?
LUN 0 is created on mkdir and cannot be removed by user so the memory
for it is allocated together with fsg_opts structure. Rest of LUNs are
being created by explicit user action and malloc() separately. This
array is used to store their pointers. To make the code easier we simply:
lun_opts[0] = &fsg_opts->lun0;
so later we don't care which lun we are dealing with.
--
Best regards,
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics
^ permalink raw reply
* Re: [PATCH RFCv2 00/21] Drivers: hv: utils: re-implement the kernel/userspace communication layer
From: Vitaly Kuznetsov @ 2015-04-08 16:00 UTC (permalink / raw)
To: KY Srinivasan
Cc: devel@linuxdriverproject.org, Haiyang Zhang,
linux-kernel@vger.kernel.org, Dexuan Cui, Radim Krcmar,
Greg Kroah-Hartman, linux-api@vger.kernel.org
In-Reply-To: <BY2PR0301MB0711A355F6C97C96DD4883C9A0020-swgC6WJTr6EX4BWLOCZ7lJwN6zqB+hSMnBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org>
KY Srinivasan <kys-0li6OtcxBFHby3iVrkZq2A@public.gmane.org> writes:
>> -----Original Message-----
>> From: Vitaly Kuznetsov [mailto:vkuznets-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org]
>> Sent: Wednesday, March 11, 2015 6:29 AM
>> To: KY Srinivasan; devel-tBiZLqfeLfOHmIFyCCdPziST3g8Odh+X@public.gmane.org
>> Cc: Haiyang Zhang; linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Dexuan Cui; Radim Krcmar;
>> Greg Kroah-Hartman; linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> Subject: [PATCH RFCv2 00/21] Drivers: hv: utils: re-implement the
>> kernel/userspace communication layer
>>
>> Changes in RFCv2:
>> - Preserve backwards compatibility with netlink-speaking daemons. [K. Y.
>> Srinivasan]
>> - Introduce transport abstraction layer. [K. Y. Srinivasan]
>> - Get rid of ioctls [Radim Krcmar]
>> - Make the series reviewable by splitting it into smaller patches.
>>
>> Anatomy of the series:
>> Patches 01 - 07 are cleanup with minor functional change.
>> Patch 08 defines the state machine.
>> Patches 09-11 convert all 3 drivers to using the state machine.
>> Patch 12 fixes a bug in fcopy. This change is going away in Patch 15, I just
>> want to highlight the fix.
>> Patch 13 introduces a transport abstraction.
>> Patch 14-16 convert all drivers to using the transport abstraction.
>> Patches 17-18 switch KVP and VSS daemon to using char devices.
>> Patches 19-20 convert FCOPY and VSS to hull handshake (the same we have
>> in KVP). These two can be postponed till we really need to distinguish
>> between different kernels in the daemon code.
>> Patch 21 unifies log messages on daemons connect across all drivers and
>> moves these messages to debug level.
>>
>> I smoke-tested this series with both old (netlink) and new (char devices)
>> daemons and tested the daemon upgrade procedure.
>>
>> Original description:
>> This series converts kvp/vss daemons to use misc char devices instead of
>> netlink for userspace/kernel communication and then updates fcopy to be
>> consistent with kvp/vss.
>>
>> Userspace/kernel communication via netlink has a number of issues:
>> - It is hard for userspace to figure out if the kernel part was loaded or not
>> and this fact can change as there is a way to enable/disable the service from
>> host side. Racy daemon startup is also a problem.
>> - When the userspace daemon restarts/dies kernel part doesn't receive a
>> notification.
>> - Netlink communication is not stable under heavy load.
>> - ...
>>
>> Vitaly Kuznetsov (21):
>> Drivers: hv: util: move kvp/vss function declarations to
>> hyperv_vmbus.h
>> Drivers: hv: kvp: reset kvp_context
>> Drivers: hv: kvp: move poll_channel() to hyperv_vmbus.h
>> Drivers: hv: fcopy: process deferred messages when we complete the
>> transaction
>> Drivers: hv: vss: process deferred messages when we complete the
>> transaction
>> Drivers: hv: kvp: rename kvp_work -> kvp_timeout_work
>> Drivers: hv: fcopy: rename fcopy_work -> fcopy_timeout_work
>> Drivers: hv: util: introduce state machine for util drivers
>> Drivers: hv: kvp: switch to using the hvutil_device_state state
>> machine
>> Drivers: hv: vss: switch to using the hvutil_device_state state
>> machine
>> Drivers: hv: fcopy: switch to using the hvutil_device_state state
>> machine
>> Drivers: hv: fcopy: set .owner reference for file operations
>> Drivers: hv: util: introduce hv_utils_transport abstraction
>> Drivers: hv: vss: convert to hv_utils_transport
>> Drivers: hv: fcopy: convert to hv_utils_transport
>> Drivers: hv: kvp: convert to hv_utils_transport
>> Tools: hv: kvp: use misc char device to communicate with kernel
>> Tools: hv: vss: use misc char device to communicate with kernel
>> Drivers: hv: vss: full handshake support
>> Drivers: hv: fcopy: full handshake support
>> Drivers: hv: utils: unify driver registration reporting
>>
>> drivers/hv/Makefile | 2 +-
>> drivers/hv/hv_fcopy.c | 287 ++++++++++++++--------------------------
>> drivers/hv/hv_kvp.c | 192 +++++++++++++--------------
>> drivers/hv/hv_snapshot.c | 168 +++++++++++++++--------
>> drivers/hv/hv_utils_transport.c | 276
>> ++++++++++++++++++++++++++++++++++++++
>> drivers/hv/hv_utils_transport.h | 51 +++++++
>> drivers/hv/hyperv_vmbus.h | 29 ++++
>> include/linux/hyperv.h | 8 --
>> include/uapi/linux/hyperv.h | 8 +-
>> tools/hv/hv_fcopy_daemon.c | 15 +++
>> tools/hv/hv_kvp_daemon.c | 166 +++++------------------
>> tools/hv/hv_vss_daemon.c | 149 ++++++---------------
>> 12 files changed, 752 insertions(+), 599 deletions(-) create mode 100644
>> drivers/hv/hv_utils_transport.c create mode 100644
>> drivers/hv/hv_utils_transport.h
>
> Vitaly,
>
> Thank you very much for taking on this project; very well done. I have mostly reviewed the code and I should be
> done shortly. Also, I am going to test this code as well. If there are no issues, I will send this out to Greg in my next
> installment of patches.
Hi K.Y.,
I don't mean to rush or anything but please let me know if I need to do
something with this series (e.g. fix something, rebase/resend it,...) to
get it applied.
Thanks,
--
Vitaly
^ permalink raw reply
* RE: [PATCH RFCv2 00/21] Drivers: hv: utils: re-implement the kernel/userspace communication layer
From: KY Srinivasan @ 2015-04-08 16:15 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: devel@linuxdriverproject.org, Haiyang Zhang,
linux-kernel@vger.kernel.org, Dexuan Cui, Radim Krcmar,
Greg Kroah-Hartman, linux-api@vger.kernel.org
In-Reply-To: <87iod64nkm.fsf@vitty.brq.redhat.com>
> -----Original Message-----
> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> Sent: Wednesday, April 8, 2015 9:01 AM
> To: KY Srinivasan
> Cc: devel@linuxdriverproject.org; Haiyang Zhang; linux-
> kernel@vger.kernel.org; Dexuan Cui; Radim Krcmar; Greg Kroah-Hartman;
> linux-api@vger.kernel.org
> Subject: Re: [PATCH RFCv2 00/21] Drivers: hv: utils: re-implement the
> kernel/userspace communication layer
>
> KY Srinivasan <kys@microsoft.com> writes:
>
> >> -----Original Message-----
> >> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> >> Sent: Wednesday, March 11, 2015 6:29 AM
> >> To: KY Srinivasan; devel@linuxdriverproject.org
> >> Cc: Haiyang Zhang; linux-kernel@vger.kernel.org; Dexuan Cui; Radim
> Krcmar;
> >> Greg Kroah-Hartman; linux-api@vger.kernel.org
> >> Subject: [PATCH RFCv2 00/21] Drivers: hv: utils: re-implement the
> >> kernel/userspace communication layer
> >>
> >> Changes in RFCv2:
> >> - Preserve backwards compatibility with netlink-speaking daemons. [K. Y.
> >> Srinivasan]
> >> - Introduce transport abstraction layer. [K. Y. Srinivasan]
> >> - Get rid of ioctls [Radim Krcmar]
> >> - Make the series reviewable by splitting it into smaller patches.
> >>
> >> Anatomy of the series:
> >> Patches 01 - 07 are cleanup with minor functional change.
> >> Patch 08 defines the state machine.
> >> Patches 09-11 convert all 3 drivers to using the state machine.
> >> Patch 12 fixes a bug in fcopy. This change is going away in Patch 15, I just
> >> want to highlight the fix.
> >> Patch 13 introduces a transport abstraction.
> >> Patch 14-16 convert all drivers to using the transport abstraction.
> >> Patches 17-18 switch KVP and VSS daemon to using char devices.
> >> Patches 19-20 convert FCOPY and VSS to hull handshake (the same we
> have
> >> in KVP). These two can be postponed till we really need to distinguish
> >> between different kernels in the daemon code.
> >> Patch 21 unifies log messages on daemons connect across all drivers and
> >> moves these messages to debug level.
> >>
> >> I smoke-tested this series with both old (netlink) and new (char devices)
> >> daemons and tested the daemon upgrade procedure.
> >>
> >> Original description:
> >> This series converts kvp/vss daemons to use misc char devices instead of
> >> netlink for userspace/kernel communication and then updates fcopy to
> be
> >> consistent with kvp/vss.
> >>
> >> Userspace/kernel communication via netlink has a number of issues:
> >> - It is hard for userspace to figure out if the kernel part was loaded or not
> >> and this fact can change as there is a way to enable/disable the service
> from
> >> host side. Racy daemon startup is also a problem.
> >> - When the userspace daemon restarts/dies kernel part doesn't receive a
> >> notification.
> >> - Netlink communication is not stable under heavy load.
> >> - ...
> >>
> >> Vitaly Kuznetsov (21):
> >> Drivers: hv: util: move kvp/vss function declarations to
> >> hyperv_vmbus.h
> >> Drivers: hv: kvp: reset kvp_context
> >> Drivers: hv: kvp: move poll_channel() to hyperv_vmbus.h
> >> Drivers: hv: fcopy: process deferred messages when we complete the
> >> transaction
> >> Drivers: hv: vss: process deferred messages when we complete the
> >> transaction
> >> Drivers: hv: kvp: rename kvp_work -> kvp_timeout_work
> >> Drivers: hv: fcopy: rename fcopy_work -> fcopy_timeout_work
> >> Drivers: hv: util: introduce state machine for util drivers
> >> Drivers: hv: kvp: switch to using the hvutil_device_state state
> >> machine
> >> Drivers: hv: vss: switch to using the hvutil_device_state state
> >> machine
> >> Drivers: hv: fcopy: switch to using the hvutil_device_state state
> >> machine
> >> Drivers: hv: fcopy: set .owner reference for file operations
> >> Drivers: hv: util: introduce hv_utils_transport abstraction
> >> Drivers: hv: vss: convert to hv_utils_transport
> >> Drivers: hv: fcopy: convert to hv_utils_transport
> >> Drivers: hv: kvp: convert to hv_utils_transport
> >> Tools: hv: kvp: use misc char device to communicate with kernel
> >> Tools: hv: vss: use misc char device to communicate with kernel
> >> Drivers: hv: vss: full handshake support
> >> Drivers: hv: fcopy: full handshake support
> >> Drivers: hv: utils: unify driver registration reporting
> >>
> >> drivers/hv/Makefile | 2 +-
> >> drivers/hv/hv_fcopy.c | 287 ++++++++++++++--------------------------
> >> drivers/hv/hv_kvp.c | 192 +++++++++++++--------------
> >> drivers/hv/hv_snapshot.c | 168 +++++++++++++++--------
> >> drivers/hv/hv_utils_transport.c | 276
> >> ++++++++++++++++++++++++++++++++++++++
> >> drivers/hv/hv_utils_transport.h | 51 +++++++
> >> drivers/hv/hyperv_vmbus.h | 29 ++++
> >> include/linux/hyperv.h | 8 --
> >> include/uapi/linux/hyperv.h | 8 +-
> >> tools/hv/hv_fcopy_daemon.c | 15 +++
> >> tools/hv/hv_kvp_daemon.c | 166 +++++------------------
> >> tools/hv/hv_vss_daemon.c | 149 ++++++---------------
> >> 12 files changed, 752 insertions(+), 599 deletions(-) create mode 100644
> >> drivers/hv/hv_utils_transport.c create mode 100644
> >> drivers/hv/hv_utils_transport.h
> >
> > Vitaly,
> >
> > Thank you very much for taking on this project; very well done. I have
> mostly reviewed the code and I should be
> > done shortly. Also, I am going to test this code as well. If there are no
> issues, I will send this out to Greg in my next
> > installment of patches.
>
> Hi K.Y.,
>
> I don't mean to rush or anything but please let me know if I need to do
> something with this series (e.g. fix something, rebase/resend it,...) to
> get it applied.
Sorry for the delay Vitaly. I finally was able to test it; please resend the patch-set that can apply on the current tip.
Regards,
K. Y
>
> Thanks,
>
> --
> Vitaly
^ permalink raw reply
* Re: [PATCH v4 00/14] Add kdbus implementation
From: Andy Lutomirski @ 2015-04-08 22:38 UTC (permalink / raw)
To: David Herrmann
Cc: Tom Gundersen, Greg Kroah-Hartman, Arnd Bergmann,
Eric W. Biederman, One Thousand Gnomes, Jiri Kosina, Linux API,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Daniel Mack,
Djalal Harouni
In-Reply-To: <CANq1E4QSja-KhVL2p-d406uLJT600zMpK2UAcwsb0LkMwa8F7w-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
[Trying again due to HTML mail goof. Trimming and responding better, too.]
On Fri, Apr 3, 2015 at 4:51 AM, David Herrmann <dh.herrmann-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Hi
>
> On Tue, Mar 31, 2015 at 8:29 PM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
>> On Tue, Mar 31, 2015 at 8:10 AM, Tom Gundersen <teg-B22kvLQNl6c@public.gmane.org> wrote:
>>> On Tue, Mar 31, 2015 at 3:58 PM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
>>>>
>>>> IOW you're taking something that you dislike aspects of and shoving
>>>> most of it in the kernel. That guarantees us an API in the kernel
>>>> that even the creators don't really like. This is, IMO, very
>>>> unfortunate.
>>>
>>> This is a misrepresentation of what David wrote. We do want this API
>>> regardless of dbus1 compatibility, but compatibility is by itself a
>>> sufficient motivation. A further motivation is reliable introspection,
>>> since this meta-data allows listing current peers on the bus and
>>> showing their identities. That's hugely useful to make the bus
>>> transparent to admins.
>>
>> I've heard the following use cases for per-connection metadata:
>>
>> - Authenticating to dbus1 services.
>
> Not necessarily authentication, but we need to support the legacy API,
> for whatever reason it was used by old applications. But..
>
>> - Identifying connected users for admin diagnostics.
>>
>> I've heard the following use cases for per-message metadata:
>>
>> - Logging.
>>
>> - Authenticating to kdbus services that want this style of authentication.
>
> ..please note that authentication on DBus has always been done with
> per-message metadata (see polkit history). However, this had to be
> reverted some years ago as it is racy (it used /proc for that, which
> can be exploited by exec'ing setuid binaries). However, the
> per-message metadata authentication worked very well for _years_
> (minus the race..), so this is already a well-established scheme. With
> kdbus we can finally implement this in a race-free manner.
>
> [...]
>> This is simply not okay for a modern interface, and in my opinion the
>> kernel should not carry code to support new APIs with weakly defined
>> security semantics. It's important that one be able to tell what the
>> security implications of one's code is without cross-referencing with
>> the implementation of the server's you're interacting with.
>
> Again, I disagree. Our concepts are established and used on UDS and
> DBus for decades.
SO_PASSCRED does not justify anything in my book. It was a mistake
and it remains a minor disaster. Please don't use it as justification
for something being a good idea.
Similarly, the fact that the *receiver* chooses which of SO_PASSCRED
and SO_PEERCRED to use is awful.
>
> Yes, we provide two ways to retrieve metadata, but the kernel offers
> several more paths to gather that information. Just because those APIs
> are available does not mean they should be used for authentication. We
> mandate per-message metadata. If applications use per-connection
> metadata, /proc, netlink, or random data, they're doing it wrong.
ISTM kdbus is trying to add three more interfaces, at least two of
which are also doing it wrong. (Which two is debatable.)
>
> Furthermore, dbus provides pretty complete and straightforward
> libraries which hide that from you. If you use glib, qt or sd-bus, you
> don't even need to deal with all that.
Libraries can't hide the issue of whether:
init_my_favorite_library();
connect_to_thingy();
setresuid(nobody);
is secure. It is either secure, insecure, or ambiguous.
Unfortunately, kdbus as currently proposed is aiming for ambiguous.
>
>> To top that off, the kdbus policy mechanism has truly bizarre effects
>> with respect to services that have unique ids and well-known names.
>> That, too, is apparently for compatibility.
>>
>> This all feels to me like a total of about four people are going to
>> understand the tangle that is kdbus security, and that's bad. I think
>> that the kernel should insist that new security-critical mechanisms
>> make sense and be hard to misuse. The current design of kdbus, in
>> contrast, feel like it will be very hard to use correctly.
>
> Native kdbus clients are authenticated with their credentials at time
> of method call. Legacy clients will always have their credentials at
> time of connect in effect. No fallbacks, no choices. It's a simple
> question whether it's a legacy client or not.
> Sounds simple to me.
I had the distinct impression that the kdbus-client-to-dbus1-server
proxy used kdbus clients' connection metadata. I could be wrong here.
--Andy
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox