Re: [PATCH 09/15] virtio-blk: Pass attribute group to device_add_disk

[Michael S. Tsirkin]

On Wed, Aug 17, 2016 at 03:15:09PM +0800, Fam Zheng wrote:
> Previously after device_add_disk returns, the KOBJ_ADD uevent is already
> emitted. Adding attributes after that is a poor usage of kobject, and
> in practice may result in race conditions with userspace, for
> example udev checks availability of certain attributes and initializes
> /dev entries conditionally.
> 
> device_add_disk can handle adding attribute group better, so use it.
> 
> Meanwhile, handle error of device_add_disk.
> 
> Signed-off-by: Fam Zheng 

Feel free to merge this with the rest of patchset
Acked-by: Michael S. Tsirkin 



> ---
>  drivers/block/virtio_blk.c | 38 +++---
>  1 file changed, 27 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 4564df5..ff60d82 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -522,10 +522,10 @@ virtblk_cache_type_show(struct device *dev, struct 
> device_attribute *attr,
>   return snprintf(buf, 40, "%s\n", virtblk_cache_types[writeback]);
>  }
>  
> -static const struct device_attribute dev_attr_cache_type_ro =
> +static struct device_attribute dev_attr_cache_type_ro =
>   __ATTR(cache_type, S_IRUGO,
>  virtblk_cache_type_show, NULL);
> -static const struct device_attribute dev_attr_cache_type_rw =
> +static struct device_attribute dev_attr_cache_type_rw =
>   __ATTR(cache_type, S_IRUGO|S_IWUSR,
>  virtblk_cache_type_show, virtblk_cache_type_store);
>  
> @@ -550,6 +550,26 @@ static struct blk_mq_ops virtio_mq_ops = {
>  static unsigned int virtblk_queue_depth;
>  module_param_named(queue_depth, virtblk_queue_depth, uint, 0444);
>  
> +static struct attribute *virtblk_attrs_ro[] = {
> + _attr_serial.attr,
> + _attr_cache_type_ro.attr,
> + NULL
> +};
> +
> +static struct attribute *virtblk_attrs_rw[] = {
> + _attr_serial.attr,
> + _attr_cache_type_rw.attr,
> + NULL
> +};
> +
> +static struct attribute_group virtblk_attr_group_ro = {
> + .attrs  = virtblk_attrs_ro,
> +};
> +
> +static struct attribute_group virtblk_attr_group_rw = {
> + .attrs  = virtblk_attrs_rw,
> +};
> +
>  static int virtblk_probe(struct virtio_device *vdev)
>  {
>   struct virtio_blk *vblk;
> @@ -560,6 +580,7 @@ static int virtblk_probe(struct virtio_device *vdev)
>   u32 v, blk_size, sg_elems, opt_io_size;
>   u16 min_io_size;
>   u8 physical_block_exp, alignment_offset;
> + struct attribute_group *attr_group;
>  
>   if (!vdev->config->get) {
>   dev_err(>dev, "%s failure: config access disabled\n",
> @@ -719,19 +740,14 @@ static int virtblk_probe(struct virtio_device *vdev)
>  
>   virtio_device_ready(vdev);
>  
> - device_add_disk(>dev, vblk->disk, NULL);
> - err = device_create_file(disk_to_dev(vblk->disk), _attr_serial);
> - if (err)
> - goto out_del_disk;
> -
>   if (virtio_has_feature(vdev, VIRTIO_BLK_F_CONFIG_WCE))
> - err = device_create_file(disk_to_dev(vblk->disk),
> -  _attr_cache_type_rw);
> + attr_group = _attr_group_rw;
>   else
> - err = device_create_file(disk_to_dev(vblk->disk),
> -  _attr_cache_type_ro);
> + attr_group = _attr_group_ro;
> + err = device_add_disk(>dev, vblk->disk, attr_group);
>   if (err)
>   goto out_del_disk;
> +
>   return 0;
>  
>  out_del_disk:
> -- 
> 2.7.4

Re: [RFC] fs: add userspace critical mounts event support

[Dmitry Torokhov]

On Sat, Sep 3, 2016 at 11:01 AM, Linus Torvalds
 wrote:
> On Sat, Sep 3, 2016 at 10:49 AM, Dmitry Torokhov
>  wrote:
>>
>> Unfortunately module loading and availability of firmware is very
>> loosely coupled.
>
> The whole "let's add a new magical proc entry to say that all
> filesystems are mounted" is all about the user space coupling them.

I will be first to say that the proposed *implementation* is nowhere
near what should be accepted. I was thinking if we kernel could post
"conditions" (maybe simple stings) that it waits for, and userspace
could unlock these "conditions". One of them might be "firmware
available".

>
> I'm just saying that if user space must know about when firmware is
> ready to be loaded anyway, just do it rigth. Not with some hacky "you
> can now do random things" flag. But by having user space actually say
> "put this module together with this firmware".
>
> If you just put the two pieces together, then the module "will just work".
>
> And quite frankly, that sounds like a much better maintenance practice
> anyway. If some module doesn't work without the firmware, then dammit,
> the two *should* go together. Putting them in two different places
> would be *INSANE*.

Quite often it does until it does not. Most of the touch controllers
work just fine until some event (abrupt cutting of power for example)
where nvram gets corrupted and they come up in bootloader mode. It is
just an example.

Thanks.

-- 
Dmitry

Re: [RFC] fs: add userspace critical mounts event support

[Linus Torvalds]

On Sat, Sep 3, 2016 at 10:49 AM, Dmitry Torokhov
 wrote:
>
> Unfortunately module loading and availability of firmware is very
> loosely coupled.

The whole "let's add a new magical proc entry to say that all
filesystems are mounted" is all about the user space coupling them.

I'm just saying that if user space must know about when firmware is
ready to be loaded anyway, just do it rigth. Not with some hacky "you
can now do random things" flag. But by having user space actually say
"put this module together with this firmware".

If you just put the two pieces together, then the module "will just work".

And quite frankly, that sounds like a much better maintenance practice
anyway. If some module doesn't work without the firmware, then dammit,
the two *should* go together. Putting them in two different places
would be *INSANE*.

Linus

Re: [RFC] fs: add userspace critical mounts event support

[Dmitry Torokhov]

On Fri, Sep 02, 2016 at 09:41:18PM -0700, Linus Torvalds wrote:
> On Sep 2, 2016 9:20 PM, "Dmitry Torokhov"  wrote:
> >
> > Like what? Some devices do need to have firmware loaded so we know
> > their capabilities, so we really can't push the firmware loading into
> > "open".
> 
> So you
> (a) document that

Document that device may come up half-broken? Not sure how that would
help end user.

> (b) make the driver only build as a module

Unfortunately module loading and availability of firmware is very
loosely coupled. Of course, if you only load modules from the same
partition that your firmware is on you can get away with it, but if some
of the modules are in initramfs and firmware is on final root fs then
it still does not work. And populating also initramfs with firmware that
might be used once in a 1000 boots is somewhat wasteful. That is not
talking about systems that do not wish to use modules for one reason or
another, or even more esoteric setups where non-essential for boot
firmware can be mounted later over nfs, etc, etc.

> (c) make sure the module and the firmware go together

I do not think it is always possible. Quite often it is though, at the
expense of increasing kernel/initramfs size.

> 
> End of problem.
> 
> Why make up random interfaces for crazy stuff?

Because we want a solution that works well for all cases, simple and
complex. This includes allowing drivers to be built into the kernel but
allow them waiting for additional data (config/firmware) that may become
available later in the game. We just need to be able to tell them when
it does not make sense to wait anymore as the data they want is not
coming, and do it more reliably then simply declaring 10 or 30 or 300
seconds time out.

Thanks.

-- 
Dmitry

Re: [Ksummit-discuss] [CORE TOPIC] (group) maintainership models

[Michael Ellerman]

On 3 September 2016 06:26:08 GMT+10:00, Linus Torvalds 
 wrote:
>On Fri, Sep 2, 2016 at 1:06 PM, Arnd Bergmann  wrote:
>>
>> When I once looked, I thought all drivers using NO_IRQ were specific
>> to powerpc or one of the less common architectures.
>
>powerpc definitely does seem to be the biggest case, with about half
>the instances of NO_IRQ being under arch/powerpc/ (and a few more in
>ppc-specific drivers).
>
>Adding the powerpc maintainers to the list - because it would really
>be nice to get rid of it, or at least make it *so* rare that we don't
>have people re-introducing it again because they thought it was the
>right thing to do.
>
>A fair amount of of it could even be done by some trivial scripting.
>Something like
>
>  git grep -wl NO_IRQ arch/powerpc/ | while read a
>  do
>  sed 's/(\([a-z_]*irq\) != NO_IRQ)/(\1)/' < $a > $a.new
>  sed 's/(\([a-z_]*irq\) == NO_IRQ)/(!\1)/' < $a.new > $a
>  done
>
>does fix at least a few of the cases. It still leaves several
>assignments and "return NO_IRQ;" statements, but a few more
>sed-scripts would take care of most of it. Then remove the #define,
>and do a full build to find any straggling cases.
>
>Michael? Ben?

Yeah sounds good. I'll do a patch on Monday and push it through the build farm.

cheers

-- 
Sent from my Android phone with K-9 Mail. Please excuse my brevity.

Re: [PATCH 1/3] powerpc/mm: Don't alias user region to other regions below PAGE_OFFSET

[Paul Mackerras]

On Fri, Sep 02, 2016 at 05:52:16PM +0530, Aneesh Kumar K.V wrote:
> 
> Hi Paul,
> 
> Really nice catch. Was this found by code analysis or do we have any
> reported issue around this ?

I found it by code analysis.

I haven't been able to find any really bad consequence, beyond leaking
some information about kernel memory.  Can you find any worse
consequence?

Paul.

Re: [RFC] fs: add userspace critical mounts event support

[Linus Torvalds]

On Sep 2, 2016 9:20 PM, "Dmitry Torokhov"  wrote:
>
> Like what? Some devices do need to have firmware loaded so we know
> their capabilities, so we really can't push the firmware loading into
> "open".

So you
(a) document that
(b) make the driver only build as a module
(c) make sure the module and the firmware go together

End of problem.

Why make up random interfaces for crazy stuff?

 Linus