Re: [PATCH 1/6] virtio: wrap find_vqs

[Cornelia Huck]

On Wed, 29 Mar 2017 23:48:44 +0300
"Michael S. Tsirkin"  wrote:

> We are going to add more parameters to find_vqs, let's wrap the call so
> we don't need to tweak all drivers every time.
> 
> Signed-off-by: Michael S. Tsirkin 
> ---
>  drivers/block/virtio_blk.c | 3 +--
>  drivers/char/virtio_console.c  | 6 +++---
>  drivers/crypto/virtio/virtio_crypto_core.c | 3 +--
>  drivers/gpu/drm/virtio/virtgpu_kms.c   | 3 +--
>  drivers/net/caif/caif_virtio.c | 3 +--
>  drivers/net/virtio_net.c   | 3 +--
>  drivers/rpmsg/virtio_rpmsg_bus.c   | 2 +-
>  drivers/scsi/virtio_scsi.c | 3 +--
>  drivers/virtio/virtio_balloon.c| 3 +--
>  drivers/virtio/virtio_input.c  | 3 +--
>  include/linux/virtio_config.h  | 9 +
>  net/vmw_vsock/virtio_transport.c   | 6 +++---
>  12 files changed, 24 insertions(+), 23 deletions(-)

Regardless whether that context thing is the right thing to do, this
looks like a sensible cleanup.

Re: [PATCH] linux/types.h: enable endian checks for all sparse builds

[Cornelia Huck]

On Thu, 8 Dec 2016 04:29:39 +0200
"Michael S. Tsirkin"  wrote:

> By now, linux is mostly endian-clean. Enabling endian-ness
> checks for everyone produces about 200 new sparse warnings for me -
> less than 10% over the 2000 sparse warnings already there.

Out of curiousity: Where do most of those warnings show up?

> 
> Not a big deal, OTOH enabling this helps people notice
> they are introducing new bugs.
> 
> So let's just drop __CHECK_ENDIAN__. Follow-up patches
> can drop distinction between __bitwise and __bitwise__.
> 
> Cc: Linus Torvalds 
> Suggested-by: Christoph Hellwig 
> Signed-off-by: Michael S. Tsirkin 
> ---
> 
> Linus, could you ack this for upstream? If yes I'll
> merge through my tree as a replacement for enabling
> this just for virtio.
> 
>  include/uapi/linux/types.h | 4 
>  1 file changed, 4 deletions(-)
> 
> diff --git a/include/uapi/linux/types.h b/include/uapi/linux/types.h
> index acf0979..41e5914 100644
> --- a/include/uapi/linux/types.h
> +++ b/include/uapi/linux/types.h
> @@ -23,11 +23,7 @@
>  #else
>  #define __bitwise__
>  #endif
> -#ifdef __CHECK_ENDIAN__
>  #define __bitwise __bitwise__
> -#else
> -#define __bitwise
> -#endif
> 
>  typedef __u16 __bitwise __le16;
>  typedef __u16 __bitwise __be16;

FWIW, I like this better than just enabling it for the virtio code.

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 42/46] virtio_scsi: v1.0 support

[Cornelia Huck]

On Sun, 30 Nov 2014 17:12:47 +0200
Michael S. Tsirkin m...@redhat.com wrote:

 Note: for consistency, and to avoid sparse errors,
   convert all fields, even those no longer in use
   for virtio v1.0.
 
 Signed-off-by: Michael S. Tsirkin m...@redhat.com
 Acked-by: Paolo Bonzini pbonz...@redhat.com
 ---
  include/linux/virtio_scsi.h | 32 +++-
  drivers/scsi/virtio_scsi.c  | 51 
 -
  2 files changed, 49 insertions(+), 34 deletions(-)
 

 @@ -196,10 +196,13 @@ static void virtscsi_complete_cmd(struct virtio_scsi 
 *vscsi, void *buf)
   break;
   }
 
 - WARN_ON(resp-sense_len  VIRTIO_SCSI_SENSE_SIZE);
 + WARN_ON(virtio32_to_cpu(vscsi-vdev, resp-sense_len) 
 + VIRTIO_SCSI_SENSE_SIZE);

Introduce a local variable for this? Might make this statement and the
min_t statement below easier to read.

   if (sc-sense_buffer) {
   memcpy(sc-sense_buffer, resp-sense,
 -min_t(u32, resp-sense_len, VIRTIO_SCSI_SENSE_SIZE));
 +min_t(u32,
 +  virtio32_to_cpu(vscsi-vdev, resp-sense_len),
 +  VIRTIO_SCSI_SENSE_SIZE));
   if (resp-sense_len)
   set_driver_byte(sc, DRIVER_SENSE);
   }

Otherwise looks good to me.

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 42/46] virtio_scsi: v1.0 support

[Cornelia Huck]

On Mon, 1 Dec 2014 14:53:05 +0200
Michael S. Tsirkin m...@redhat.com wrote:

 On Mon, Dec 01, 2014 at 01:50:01PM +0100, Cornelia Huck wrote:
  On Sun, 30 Nov 2014 17:12:47 +0200
  Michael S. Tsirkin m...@redhat.com wrote:
  
   Note: for consistency, and to avoid sparse errors,
 convert all fields, even those no longer in use
 for virtio v1.0.
   
   Signed-off-by: Michael S. Tsirkin m...@redhat.com
   Acked-by: Paolo Bonzini pbonz...@redhat.com
   ---
include/linux/virtio_scsi.h | 32 +++-
drivers/scsi/virtio_scsi.c  | 51 
   -
2 files changed, 49 insertions(+), 34 deletions(-)
   
  
   @@ -196,10 +196,13 @@ static void virtscsi_complete_cmd(struct 
   virtio_scsi *vscsi, void *buf)
 break;
 }
   
   - WARN_ON(resp-sense_len  VIRTIO_SCSI_SENSE_SIZE);
   + WARN_ON(virtio32_to_cpu(vscsi-vdev, resp-sense_len) 
   + VIRTIO_SCSI_SENSE_SIZE);
  
  Introduce a local variable for this? Might make this statement and the
  min_t statement below easier to read.
 
 I prefer 1:1 code conversions. We can do refactorings on top.
 Since you mention this line as hard to read, I'll just make it  80
 chars for now, and trivial line splits can come later.
 OK?

I guess I don't care strongly enough about this, so go ahead.

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 38/42] virtio_scsi: v1.0 support

[Cornelia Huck]

On Tue, 25 Nov 2014 18:44:08 +0200
Michael S. Tsirkin m...@redhat.com wrote:

 Note: for consistency, and to avoid sparse errors,
   convert all fields, even those no longer in use
   for virtio v1.0.
 
 Signed-off-by: Michael S. Tsirkin m...@redhat.com
 ---
  include/linux/virtio_scsi.h | 32 +++-
  drivers/scsi/virtio_scsi.c  | 51 
 -
  2 files changed, 49 insertions(+), 34 deletions(-)
 

 @@ -158,7 +158,7 @@ static void virtscsi_complete_cmd(struct virtio_scsi 
 *vscsi, void *buf)
   sc, resp-response, resp-status, resp-sense_len);
 
   sc-result = resp-status;
 - virtscsi_compute_resid(sc, resp-resid);
 + virtscsi_compute_resid(sc, __virtio32_to_cpu(vscsi-vdev, resp-resid));

Confused. Don't you need the non-underscore versions if you pass in a
vdev as first parameter?

   switch (resp-response) {
   case VIRTIO_SCSI_S_OK:
   set_host_byte(sc, DID_OK);

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 13/25] virtio_console: enable VQs early

[Cornelia Huck]

On Mon, 20 Oct 2014 13:07:50 +0100
Thomas Graf tg...@suug.ch wrote:

 On 10/13/14 at 10:50am, Michael S. Tsirkin wrote:
  virtio spec requires drivers to set DRIVER_OK before using VQs.
  This is set automatically after probe returns, virtio console violated this
  rule by adding inbufs, which causes the VQ to be used directly within
  probe.
  
  To fix, call virtio_device_ready before using VQs.
  
  Signed-off-by: Michael S. Tsirkin m...@redhat.com
  ---
   drivers/char/virtio_console.c | 2 ++
   1 file changed, 2 insertions(+)
  
  diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
  index b585b47..6ebe8f6 100644
  --- a/drivers/char/virtio_console.c
  +++ b/drivers/char/virtio_console.c
  @@ -1449,6 +1449,8 @@ static int add_port(struct ports_device *portdev, u32 
  id)
  spin_lock_init(port-outvq_lock);
  init_waitqueue_head(port-waitqueue);
   
  +   virtio_device_ready(portdev-vdev);
  +
  /* Fill the in_vq with buffers so the host can send us data. */
  nr_added_bufs = fill_queue(port-in_vq, port-inbuf_lock);
  if (!nr_added_bufs) {
 
 Seems like probe and add_port() now both set VIRTIO_CONFIG_S_DRIVER_OK

I think we need to set this in the probe function instead, otherwise we
fail for multiqueue (which also wants to use the control queue early).

Completely untested patch below; I can send this with proper s-o-b if
it helps.

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index bfa6400..cf7a561 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1449,8 +1449,6 @@ static int add_port(struct ports_device *portdev, u32 id)
spin_lock_init(port-outvq_lock);
init_waitqueue_head(port-waitqueue);
 
-   virtio_device_ready(portdev-vdev);
-
/* Fill the in_vq with buffers so the host can send us data. */
nr_added_bufs = fill_queue(port-in_vq, port-inbuf_lock);
if (!nr_added_bufs) {
@@ -2026,6 +2024,8 @@ static int virtcons_probe(struct virtio_device *vdev)
spin_lock_init(portdev-ports_lock);
INIT_LIST_HEAD(portdev-ports);
 
+   virtio_device_ready(portdev-vdev);
+
if (multiport) {
unsigned int nr_added_bufs;
 

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/7] convert semaphore to mutex in struct class

[Cornelia Huck]

On Fri, 11 Jan 2008 10:33:16 +0800,
Dave Young [EMAIL PROTECTED] wrote:


   +struct device *class_find_device(struct class *class, void *data,
   +int (*match)(struct device *, void *))
   +{
   + struct device *dev;
   +
   + if (!class)
   + return NULL;
   +
   + mutex_lock(class-mutex);
   + list_for_each_entry(dev, class-devices, node)
   + if (match(dev, data)  get_device(dev))
 
  First get the reference and then drop it if the match function returns
  0 to make this analogous to the other _find_device() functions?
 
 It's just like other _find_device() functions. Are these more get/put
 really needed?

The other _find_device() functions operate on klists, which means that
there is an implicit get while the element is handled. This function
operates on a normal list, which means that getting/putting the
reference looks a bit different if we want the same effect.
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/7] convert semaphore to mutex in struct class

[Cornelia Huck]

On Thu, 10 Jan 2008 17:48:43 +0800,
Dave Young [EMAIL PROTECTED] wrote:

Please add a kerneldoc comment for each of the new interfaces.

 +int class_for_each_device(struct class *class, void *data,
 +int (*fn)(struct device *, void *))
 +{
 + struct device *dev;
 + int error = 0;
 +
 + if (!class)
 + return -EINVAL;
 + mutex_lock(class-mutex);
 + list_for_each_entry(dev, class-devices, node) {
 + error = fn(dev, data);

Hm, the equivalent _for_each_device() functions all elevate the
device's refcount while calling fn(). I wonder whether we want this
here as well?

 + if (error)
 + break;
 + }
 + mutex_unlock(class-mutex);
 +
 + return error;
 +}
 +EXPORT_SYMBOL_GPL(class_for_each_device);
 +
 +struct device *class_find_device(struct class *class, void *data,
 +int (*match)(struct device *, void *))
 +{
 + struct device *dev;
 +
 + if (!class)
 + return NULL;
 +
 + mutex_lock(class-mutex);
 + list_for_each_entry(dev, class-devices, node)
 + if (match(dev, data)  get_device(dev))

First get the reference and then drop it if the match function returns
0 to make this analogous to the other _find_device() functions?

 + break;
 + mutex_unlock(class-mutex);
 +
 + return dev;
 +}
 +EXPORT_SYMBOL_GPL(class_find_device);
 +
 +int class_for_each_child(struct class *class, void *data,
 +int (*fn)(struct class_device *, void *))

Haven't looked at the callers, but isn't it better to convert them to
use struct device instead so we don't need to introduce new
class_device api?

 +{
 + struct class_device *dev;
 + int error = 0;
 +
 + if (!class)
 + return -EINVAL;
 + mutex_lock(class-mutex);
 + list_for_each_entry(dev, class-children, node) {
 + error = fn(dev, data);

Same comment as above concerning reference counts.

 + if (error)
 + break;
 + }
 + mutex_unlock(class-mutex);
 +
 + return error;
 +}
 +EXPORT_SYMBOL_GPL(class_for_each_child);
 +
 +struct class_device *class_find_child(struct class *class, void *data,
 +int (*match)(struct class_device *, void *))
 +{
 + struct class_device *dev;
 +
 + if (!class)
 + return NULL;
 +
 + mutex_lock(class-mutex);
 + list_for_each_entry(dev, class-children, node)
 + if (match(dev, data)  class_device_get(dev))

And here.

 + break;
 + mutex_unlock(class-mutex);
 +
 + return dev;
 +}
 +EXPORT_SYMBOL_GPL(class_find_child);
 +
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] sysfs: add filter function to groups

[Cornelia Huck]

On Tue, 30 Oct 2007 20:55:06 -0700,
Greg KH [EMAIL PROTECTED] wrote:

 On Tue, Oct 30, 2007 at 01:25:43PM -0500, James Bottomley wrote:
  Index: BUILD-2.6/fs/sysfs/group.c
  ===
  --- BUILD-2.6.orig/fs/sysfs/group.c 2007-10-28 17:27:04.0 -0500
  +++ BUILD-2.6/fs/sysfs/group.c  2007-10-30 12:35:47.0 -0500
  @@ -16,25 +16,31 @@
   #include sysfs.h
   
   
  -static void remove_files(struct sysfs_dirent *dir_sd,
  +static void remove_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
   const struct attribute_group *grp)
   {
  struct attribute *const* attr;
  +   int i;
   
  -   for (attr = grp-attrs; *attr; attr++)
  -   sysfs_hash_and_remove(dir_sd, (*attr)-name);
  +   for (i = 0, attr = grp-attrs; *attr; i++, attr++)
  +   if (grp-is_visible 
  +   grp-is_visible(kobj, *attr, i))
  +   sysfs_hash_and_remove(dir_sd, (*attr)-name);
 
 Hm, doesn't this break for the zillions of attribute groups that do not
 have the is_visible function set?
 
  -static int create_files(struct sysfs_dirent *dir_sd,
  +static int create_files(struct sysfs_dirent *dir_sd, struct kobject *kobj,
  const struct attribute_group *grp)
   {
  struct attribute *const* attr;
  -   int error = 0;
  +   int error = 0, i;
   
  -   for (attr = grp-attrs; *attr  !error; attr++)
  -   error = sysfs_add_file(dir_sd, *attr, SYSFS_KOBJ_ATTR);
  +   for (i = 0, attr = grp-attrs; *attr  !error; i++, attr++)
  +   if (grp-is_visible 
  +   grp-is_visible(kobj, *attr, i))
  +   error |=
  +   sysfs_add_file(dir_sd, *attr, SYSFS_KOBJ_ATTR);
 
 Same problem here, if grp-is_visible is not set, sysfs_add_file() would
 never be called, right?
 
 Other than the logic problem (I think), I have no issue with this idea
 at all.  Care to redo this so it works?

Would it make more sense then to turn the meaning of the callback
around?

for (...) {
if (grp-mask_out  grp-mask_out(kobj, *attr, i))
continue;
error |= sysfs_add_file(...);
}
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] sysfs: add filter function to groups

[Cornelia Huck]

On Wed, 31 Oct 2007 10:52:35 +0100,
Stefan Richter [EMAIL PROTECTED] wrote:

 Cornelia Huck wrote:
  Greg KH [EMAIL PROTECTED] wrote:
  On Tue, Oct 30, 2007 at 01:25:43PM -0500, James Bottomley wrote:
  + for (i = 0, attr = grp-attrs; *attr; i++, attr++)
  + if (grp-is_visible 
  + grp-is_visible(kobj, *attr, i))
  + sysfs_hash_and_remove(dir_sd, (*attr)-name);
  Hm, doesn't this break for the zillions of attribute groups that do not
  have the is_visible function set?
 ...
  Would it make more sense then to turn the meaning of the callback
  around?
  
  for (...) {
  if (grp-mask_out  grp-mask_out(kobj, *attr, i))
  continue;
  error |= sysfs_add_file(...);
  }
 
   if (!grp-is_visible ||
   grp-is_visible(kobj, *attr, i))
   add or remove();
 

Hm, I find that a bit harder to parse...

mask_out() would also imply that the common use case is to have all
attributes in the group created and that you need to take action to
have an attribute not created.
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] sysfs: add filter function to groups

[Cornelia Huck]

On Wed, 31 Oct 2007 11:37:36 +0100,
Stefan Richter [EMAIL PROTECTED] wrote:

  mask_out() would also imply that the common use case is to have all
  attributes in the group created and that you need to take action to
  have an attribute not created.
 
 Here you have a point.  But James has a point too when he says:
 | We basically want to show capability by which file is present.

Currently, if you register an attribute group, all of the attributes
will show up. I find it more intuitive to have to block some attributes
explicitely if I want to have more control, but the original logic is
fine with me as well, if most people prefer that :)

 Anyway, /if/ the reverse logic is preferred, it shouldn't be called
 mask_out() but rather is_masked() or is_hidden() or the like.

Sure. is_masked() would be fine.

-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] sysfs: add filter function to groups

[Cornelia Huck]

On Mon, 29 Oct 2007 12:24:06 -0500,
James Bottomley [EMAIL PROTECTED] wrote:

  Can you determine which subset of the attributes you want just before
  actually creating the group? Then you could do something like:
  
  create_group(grp, kobj)
  {
  grp-update_creation_mask(kobj);
  actually_create_attrs();
  }
 
 That's actually what we currently do (at least in hand coded form) in
 the current transport classes.  However, it leads to one separate group
 for each attached class.  With the filter approach, we only need one
 constructed group for every transport class.

I meant doing it in the core. You still have one group for all cases,
but immediately before creating the attributes, the core checks back
which ones it should create. (Of course, that doesn't solve your
problems if you dynamically want to change availability of attributes
later on. You would need a different mechanism for that.)
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] sysfs: add filter function to groups

[Cornelia Huck]

On Mon, 29 Oct 2007 12:29:27 -0500,
James Bottomley [EMAIL PROTECTED] wrote:

 On Mon, 2007-10-29 at 13:27 -0400, Jeff Garzik wrote:
  James Bottomley wrote:
   visibility and creation are the same thing, aren't they?  An invisible
   attribute doesn't appear in the sysfs directory, so it's equivalent to
   the file for it not being created.
  
  
  What about the case where it's visible at creation time, but then needs 
  to be made selectively invisible later on?
  
  That implies either a remove operation or dentry checks on each lookup?
 
 Yes, that comes with the bitmap manipulation code.  There will be a way
 to add and remove runtime visibility.  I just wanted to get the basic
 concept agreed to first.

But visibility always comes with creation or deletion of attributes?

Perhaps what we want is to move knowledge of the attributes to the
kobject (when attributes are created/deleted), while we decide on
visibilty of the attributes (creation/deletion of sysfs entries) based
on a filter (where visibility may change, while the attribute still
exists).
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] sysfs: add filter function to groups

[Cornelia Huck]

On Mon, 29 Oct 2007 11:57:51 -0500,
James Bottomley [EMAIL PROTECTED] wrote:

 On Mon, 2007-10-29 at 17:54 +0100, Kay Sievers wrote:
  On Mon, 2007-10-29 at 10:16 -0500, James Bottomley wrote:
   This patch is a first pass at adding a filter function to the group
   attributes, just to see how the idea flies.  If everyone's OK with this,
   I think the next thing that we might do is add bitmap functions (so
   every bit in the bitmap has a name, but also might not appear) to
   groups.
  
  Bitmaps in the attribute groups?
 
 Actually, no ... that would spoil our one group for all devices rule.
 So they would be a set of helper functions for manipulating bitmaps, but
 the bitmap would have to be in separate storage elsewhere.

Can you determine which subset of the attributes you want just before
actually creating the group? Then you could do something like:

create_group(grp, kobj)
{
grp-update_creation_mask(kobj);
actually_create_attrs();
}

 
struct attribute_group {
 const char  *name;
   + int (*filter_show)(struct kobject *, int);
  
  Are you sure that you want to return an array index here, instead of the
  actual attribute? Like:
 
 Actually, it returns a true/false value indicating whether the given
 attribute should be displayed.
 
int (*filter_show)(struct kobject *kobj, struct attribute *attr);

I'd agree that using the attribute is better in this function.

  
  The names show and store are the ususal file-operation names, and we
  are not filtering a show here, right? Maybe create, or export, or
  something else might be a better name?
 
 how about (*attribute_is_visible)?

Well, you don't only stop the visibility, but the creation of the
attribute, so perhaps (*creation_filter)?
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch 1/3] ps3: Disk Storage Driver

[Cornelia Huck]

On Thu, 19 Jul 2007 10:57:53 +0200 (CEST),
Geert Uytterhoeven [EMAIL PROTECTED] wrote:

 Were .probe()/.remove() made concurrent again? I thought that idea was dropped
 because it caused too many problems?

Well, for a given device, -probe()/-remove() are locked by dev-sem,
so that there cannot be two probes/removes for the same device at the
same time. However, if you have multiple hotplug events pending at the
same time, it depends on your bus whether it does some serialization or
whether it allows multiple probes/removes running for different devices.
(Initial probing of a bus and probing of all devices with a new driver
is done serialized again, I think that's what you're referring to.)
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch 1/3] ps3: Disk Storage Driver

[Cornelia Huck]

On Thu, 19 Jul 2007 11:36:31 +0200 (CEST),
Geert Uytterhoeven [EMAIL PROTECTED] wrote:

 We have a probe thread that checks for new storage devices and adds them to 
 the
 bus with ps3_system_bus_device_register(), which calls device_register().
 
 I guess the actual bus probe() routine gets called through the notifier call
 chain? That's where I got lost...

No, -probe() is called from device_register() directly. If you just
have one probe thread, you should have enough serialization.

(Unless you're doing something interesting from the bus_notifier, which
is called via the notifier chain before -probe()...)
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch 1/3] ps3: Disk Storage Driver

[Cornelia Huck]

On Thu, 19 Jul 2007 14:13:33 +0200 (CEST),
Geert Uytterhoeven [EMAIL PROTECTED] wrote:

 Any chance this will change? I already added a mutex to ps3disk to protect
 against this.

Probably not in the near future. A mutex looks like a good idea though,
since one never knows (and the driver core generally doesn't make many
guarantees regarding serialization).
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: 2.6.22-rc3-mm1

[Cornelia Huck]

On Thu, 31 May 2007 15:10:05 -0700,
Andrew Morton [EMAIL PROTECTED] wrote:

 Cornelia, afaict your patch has no actual delendency upon Dan's
 dma-mapping-prevent-dma-dependent-code-from-linking-on.patch, correct?  If
 so, I can merge it via James and then merge Dan's patch once James has
 merged.

AFAICS there's no dependency in that direction.

 diff -puN 
 include/scsi/scsi_cmnd.h~scsi-dont-build-scsi_dma_mapunmap-for-has_dma 
 include/scsi/scsi_cmnd.h
 --- a/include/scsi/scsi_cmnd.h~scsi-dont-build-scsi_dma_mapunmap-for-has_dma
 +++ a/include/scsi/scsi_cmnd.h
 @@ -135,8 +135,10 @@ extern void scsi_kunmap_atomic_sg(void *
  extern struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *, gfp_t);
  extern void scsi_free_sgtable(struct scatterlist *, int);
 
 +#ifdef CONFIG_SCSI_DMA
  extern int scsi_dma_map(struct scsi_cmnd *cmd);
  extern void scsi_dma_unmap(struct scsi_cmnd *cmd);
 +#endif
 
  #define scsi_sg_count(cmd) ((cmd)-use_sg)
  #define scsi_sglist(cmd) ((struct scatterlist *)(cmd)-request_buffer)
 
 We don't really need the ifdefs here.  If someone incorrectly calls these
 functions then they'll get a link-time failure anyway.  The downside of
 removing these ifdefs is that they won't get a compile-time warning, but I
 tend to think that this small cost is worth it.

OK, fine with me.
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: 2.6.22-rc3-mm1

[Cornelia Huck]

On Wed, 30 May 2007 23:58:23 -0700,
Andrew Morton [EMAIL PROTECTED] wrote:

 
 ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.22-rc3/2.6.22-rc3-mm1/

With 

 +dma-mapping-prevent-dma-dependent-code-from-linking-on.patch

scsi fails to build on !HAS_DMA architectures:

drivers/built-in.o(.text+0x20af6): In function `scsi_dma_map':
: undefined reference to `dma_map_sg'
drivers/built-in.o(.text+0x20b5c): In function `scsi_dma_unmap':
: undefined reference to `dma_unmap_sg'

I split those functions out into a new file. Builds on s390 and i386.



scsi: Don't build scsi_dma_{map,unmap} for !HAS_DMA

Move scsi_dma_{map,unmap} into scsi_lib_dma.c which is only build
if HAS_DMA is set.

Signed-off-by: Cornelia Huck [EMAIL PROTECTED]

---
 drivers/scsi/Kconfig|5 
 drivers/scsi/Makefile   |6 ++---
 drivers/scsi/scsi_lib.c |   38 -
 drivers/scsi/scsi_lib_dma.c |   50 
 include/scsi/scsi_cmnd.h|2 +
 5 files changed, 60 insertions(+), 41 deletions(-)

--- linux-2.6.orig/drivers/scsi/Kconfig
+++ linux-2.6/drivers/scsi/Kconfig
@@ -10,6 +10,7 @@ config RAID_ATTRS
 config SCSI
tristate SCSI device support
depends on BLOCK
+   select SCSI_DMA if HAS_DMA
---help---
  If you want to use a SCSI hard disk, SCSI tape drive, SCSI CD-ROM or
  any other SCSI device under Linux, say Y and make sure that you know
@@ -29,6 +30,10 @@ config SCSI
  However, do not compile this as a module if your root file system
  (the one containing the directory /) is located on a SCSI device.
 
+config SCSI_DMA
+   bool
+   default n
+
 config SCSI_TGT
tristate SCSI target support
depends on SCSI  EXPERIMENTAL
--- linux-2.6.orig/drivers/scsi/Makefile
+++ linux-2.6/drivers/scsi/Makefile
@@ -145,9 +145,9 @@ obj-$(CONFIG_SCSI_DEBUG)+= scsi_debug.o
 obj-$(CONFIG_SCSI_WAIT_SCAN)   += scsi_wait_scan.o
 
 scsi_mod-y += scsi.o hosts.o scsi_ioctl.o constants.o \
-  scsicam.o scsi_error.o scsi_lib.o \
-  scsi_scan.o scsi_sysfs.o \
-  scsi_devinfo.o
+  scsicam.o scsi_error.o scsi_lib.o
+scsi_mod-$(CONFIG_SCSI_DMA)+= scsi_lib_dma.o
+scsi_mod-y += scsi_scan.o scsi_sysfs.o scsi_devinfo.o
 scsi_mod-$(CONFIG_SCSI_NETLINK)+= scsi_netlink.o
 scsi_mod-$(CONFIG_SYSCTL)  += scsi_sysctl.o
 scsi_mod-$(CONFIG_SCSI_PROC_FS)+= scsi_proc.o
--- linux-2.6.orig/drivers/scsi/scsi_lib.c
+++ linux-2.6/drivers/scsi/scsi_lib.c
@@ -2290,41 +2290,3 @@ void scsi_kunmap_atomic_sg(void *virt)
kunmap_atomic(virt, KM_BIO_SRC_IRQ);
 }
 EXPORT_SYMBOL(scsi_kunmap_atomic_sg);
-
-/**
- * scsi_dma_map - perform DMA mapping against command's sg lists
- * @cmd:   scsi command
- *
- * Returns the number of sg lists actually used, zero if the sg lists
- * is NULL, or -ENOMEM if the mapping failed.
- */
-int scsi_dma_map(struct scsi_cmnd *cmd)
-{
-   int nseg = 0;
-
-   if (scsi_sg_count(cmd)) {
-   struct device *dev = cmd-device-host-shost_gendev.parent;
-
-   nseg = dma_map_sg(dev, scsi_sglist(cmd), scsi_sg_count(cmd),
- cmd-sc_data_direction);
-   if (unlikely(!nseg))
-   return -ENOMEM;
-   }
-   return nseg;
-}
-EXPORT_SYMBOL(scsi_dma_map);
-
-/**
- * scsi_dma_unmap - unmap command's sg lists mapped by scsi_dma_map
- * @cmd:   scsi command
- */
-void scsi_dma_unmap(struct scsi_cmnd *cmd)
-{
-   if (scsi_sg_count(cmd)) {
-   struct device *dev = cmd-device-host-shost_gendev.parent;
-
-   dma_unmap_sg(dev, scsi_sglist(cmd), scsi_sg_count(cmd),
-cmd-sc_data_direction);
-   }
-}
-EXPORT_SYMBOL(scsi_dma_unmap);
--- linux-2.6.orig/include/scsi/scsi_cmnd.h
+++ linux-2.6/include/scsi/scsi_cmnd.h
@@ -135,8 +135,10 @@ extern void scsi_kunmap_atomic_sg(void *
 extern struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *, gfp_t);
 extern void scsi_free_sgtable(struct scatterlist *, int);
 
+#ifdef CONFIG_SCSI_DMA
 extern int scsi_dma_map(struct scsi_cmnd *cmd);
 extern void scsi_dma_unmap(struct scsi_cmnd *cmd);
+#endif
 
 #define scsi_sg_count(cmd) ((cmd)-use_sg)
 #define scsi_sglist(cmd) ((struct scatterlist *)(cmd)-request_buffer)
--- /dev/null
+++ linux-2.6/drivers/scsi/scsi_lib_dma.c
@@ -0,0 +1,50 @@
+/*
+ * SCSI library functions depending on DMA
+ */
+
+#include linux/blkdev.h
+#include linux/device.h
+#include linux/kernel.h
+
+#include scsi/scsi.h
+#include scsi/scsi_cmnd.h
+#include scsi/scsi_device.h
+#include scsi/scsi_host.h
+
+/**
+ * scsi_dma_map - perform DMA mapping against command's sg lists
+ * @cmd:   scsi command
+ *
+ * Returns the number of sg lists actually used, zero if the sg lists
+ * is NULL

Re: 2.6.22-rc3-mm1

[Cornelia Huck]

On Thu, 31 May 2007 06:15:57 -0600,
Matthew Wilcox [EMAIL PROTECTED] wrote:

 On Thu, May 31, 2007 at 02:09:22PM +0200, Cornelia Huck wrote:
  I split those functions out into a new file. Builds on s390 and i386.
 
 Why not just put #ifdef CONFIG_HAS_DMA / #endif around the pair of
 functions?  I don't see the need to add a new Kconfig symbol and a new
 file for this.

I prefer a new file over #ifdefs in c files. (New dma-dependent stuff
would also have a place where it could go to.)

But I'll do whatever ends up as consensus :)
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: 2.6.22-rc3-mm1

[Cornelia Huck]

On Thu, 31 May 2007 08:35:13 -0400,
Jeff Garzik [EMAIL PROTECTED] wrote:

 Cornelia Huck wrote:
  On Thu, 31 May 2007 06:15:57 -0600,
  Matthew Wilcox [EMAIL PROTECTED] wrote:
  
  On Thu, May 31, 2007 at 02:09:22PM +0200, Cornelia Huck wrote:
  I split those functions out into a new file. Builds on s390 and i386.
  Why not just put #ifdef CONFIG_HAS_DMA / #endif around the pair of
  functions?  I don't see the need to add a new Kconfig symbol and a new
  file for this.
  
  I prefer a new file over #ifdefs in c files. (New dma-dependent stuff
  would also have a place where it could go to.)
  
  But I'll do whatever ends up as consensus :)
 
 50 lines isn't much need for a new file.

OK, so here's an alternative patch:


scsi: Don't build scsi_dma_{map,unmap} for !HAS_DMA

Use #ifdef CONFIG_HAS_DMA for the two dma-dependent functions.

Signed-off-by: Cornelia Huck [EMAIL PROTECTED]

---
 drivers/scsi/scsi_lib.c  |2 ++
 include/scsi/scsi_cmnd.h |2 ++
 2 files changed, 4 insertions(+)

--- linux-2.6.orig/drivers/scsi/scsi_lib.c
+++ linux-2.6/drivers/scsi/scsi_lib.c
@@ -2291,6 +2291,7 @@ void scsi_kunmap_atomic_sg(void *virt)
 }
 EXPORT_SYMBOL(scsi_kunmap_atomic_sg);
 
+#ifdef CONFIG_HAS_DMA
 /**
  * scsi_dma_map - perform DMA mapping against command's sg lists
  * @cmd:   scsi command
@@ -2328,3 +2329,4 @@ void scsi_dma_unmap(struct scsi_cmnd *cm
}
 }
 EXPORT_SYMBOL(scsi_dma_unmap);
+#endif
--- linux-2.6.orig/include/scsi/scsi_cmnd.h
+++ linux-2.6/include/scsi/scsi_cmnd.h
@@ -135,8 +135,10 @@ extern void scsi_kunmap_atomic_sg(void *
 extern struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *, gfp_t);
 extern void scsi_free_sgtable(struct scatterlist *, int);
 
+#ifdef CONFIG_HAS_DMA
 extern int scsi_dma_map(struct scsi_cmnd *cmd);
 extern void scsi_dma_unmap(struct scsi_cmnd *cmd);
+#endif
 
 #define scsi_sg_count(cmd) ((cmd)-use_sg)
 #define scsi_sglist(cmd) ((struct scatterlist *)(cmd)-request_buffer)
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFD driver-core] Lifetime problems of the current driver model

[Cornelia Huck]

On Mon, 2 Apr 2007 04:59:43 +0900,
Tejun Heo [EMAIL PROTECTED] wrote:

 Okay, here's preliminary patch.  I've tested it very lightly so it's
 likely to contain bugs and definitely needs to be splitted.

Cool. However, there's something fishy there (not sure whether it's in
your patch or a latent bug in the ccw bus code that just has been
uncovered):

[ cut here ]
kernel BUG at mm/slab.c:607!
illegal operation: 0001 [#1]
CPU:1Not tainted
Process kslowcrw (pid: 36, task: 01a80cc0, ksp:
01a87ce0) Krnl PSW : 040400018000 000b21b0
(kfree+0x144/0x154) R:0 T:1 IO:0 EX:0 Key:0 M:1 W:0 P:0 AS:0 CC:0 PM:0
EA:3 Krnl GPRS:   009836d8
03fffd22ffe0 000b20aa 00396ba8 01a6d100
00986c88 070001a879d8 00555428 0344a5b0
01a879c8 0344a500 00390ca8 000b20aa
01a879c8 Krnl Code: 000b21a2: e392c0480024
stg %r9,72(%r2,%r12) 000b21a8: a7f4ffc9   brc
15,1000b213a 000b21ac: a7f40001   brc 15,b21ae
  000b21b0: a7f4ff9d   brc 15,1000b20ea
   000b21b4: e3303014   lg  %r3,16(%r3)
   000b21ba: a7f4ff90   brc 15,1000b20da
   000b21be: 0707   bcr 0,%r7
   000b21c0: eb8ff0580024   stmg
%r8,%r15,88(%r15) Call Trace:
([000b20aa] kfree+0x3e/0x154)
 [001166f2] release_sysfs_dirent+0x3e/0xf4
 [00114914] sysfs_hash_and_remove+0x150/0x154
 [00118aec] remove_files+0x48/0x68
 [00118b84] sysfs_remove_group+0x78/0xe8
 [001cce3c] device_remove_groups+0x48/0x68
 [001cd4c0] device_remove_attrs+0x3c/0x7c
 [001cd70e] device_del+0x20e/0x3a8
 [001cd8d2] device_unregister+0x2a/0x44
 [0022fb2c] css_sch_device_unregister+0x3c/0x54
 [00230124] css_evaluate_subchannel+0x2f0/0x400
 [0023041a] css_trigger_slow_path+0xda/0x154
 [00054c9a] run_workqueue+0x136/0x1fc
 [00054e3a] worker_thread+0xda/0x170
 [0005b05a] kthread+0x122/0x15c
 [00019912] kernel_thread_starter+0x6/0xc
 [0001990c] kernel_thread_starter+0x0/0xc

This happens when I detach a device (which causes it to be
unregistered). I'll look at it when I'm fully operational.
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFD driver-core] Lifetime problems of the current driver model

[Cornelia Huck]

On Mon, 2 Apr 2007 11:20:48 +0200,
Cornelia Huck [EMAIL PROTECTED] wrote:

 Cool. However, there's something fishy there (not sure whether it's in
 your patch or a latent bug in the ccw bus code that just has been
 uncovered):

Similar bug when loading/unloading a module that creates a driver
attribute. The winner seems to be kfree(sd-s_element) in
release_sysfs_dirent() (in case of an attribute, it will point to the
attribute structure, which is usually statically created)...
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFD driver-core] Lifetime problems of the current driver model

[Cornelia Huck]

On Sat, 31 Mar 2007 12:12:48 +0900,
Tejun Heo [EMAIL PROTECTED] wrote:

  Hm, but as long as dk0 is registered, it can be looked up and someone
  could get a reference on it.
 
 Yeah, exactly.  That's why any getting any kobject reference backed by a
 module must be accompanied by try_module_get().
 
 int mydrv_get_dk(struct dk *dk)
 {
   rc = try_module_get(mydrv);
   if (rc)
   return rc;
   kobject_get(dk-kobj);
   return 0;
 }

This works if the caller always knows which module to grab (I was
thinking about some tree-walking code).

 Exactly, in that case, module reference count must not be zero.  You and
 I are saying the same thing.  Why are we running in circle?

I hope we're not, it just makes one dizzy :)

I'll think some more about it...
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFD driver-core] Lifetime problems of the current driver model

[Cornelia Huck]

On Fri, 30 Mar 2007 18:43:02 +0900,
Tejun Heo [EMAIL PROTECTED] wrote:

 One way to solve this problem is to subordinate lifetime rule #b to
 rule #c.  Each kobject points to its owning module such that grabbing
 a kobject automatically grabs the module.  The problem with this
 approach is that it requires wide update and makes kobject_get
 heavier.

Shouldn't getting/putting the module refcount be solely done in
kobject.c? Grab the module reference when the kobject is created and
release the module reference in kobject_cleanup() after the release
function has been called. This doesn't make kobject_get() heavier, and
it ensures we don't delete the module until after the last kobject it is
supposed to clean up has been released.
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFD driver-core] Lifetime problems of the current driver model

[Cornelia Huck]

On Fri, 30 Mar 2007 22:19:52 +0900,
Tejun Heo [EMAIL PROTECTED] wrote:

  Shouldn't getting/putting the module refcount be solely done in
  kobject.c? Grab the module reference when the kobject is created and
  release the module reference in kobject_cleanup() after the release
  function has been called. This doesn't make kobject_get() heavier, and
  it ensures we don't delete the module until after the last kobject it is
  supposed to clean up has been released.
 
 If we do that, we wouldn't be able to unload a module if there is any
 kobject referencing it even when the node has no openers, so no easy way
 out there.  :-(

But we must not unload the module when there is still a kobject around
referencing a release function in the module, or we will oops if the
kobject is finally released. If needed, the module must clean up its
kobjects in its exit function.
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFD driver-core] Lifetime problems of the current driver model

[Cornelia Huck]

On Fri, 30 Mar 2007 22:58:39 +0900,
Tejun Heo [EMAIL PROTECTED] wrote:

 It's a little bit more convoluted than that.  Module reference count of
 zero doesn't indicate that there is no one referencing the module.  It
 just means that the module can be unloaded.  ie. There still can be any
 number of kobjects with release function backed by the module but as
 long as all of them can be deleted and released by module exit function,
 the module is unloadable at that point.
 
 IOW, module reference count does not count number of objects depending
 on the module.  It counts the number of active usages of those objects.

We must make sure that the module is never deleted while there may be
calls to -release functions - the exit function can only return when
all -release calls have returned. This can be guaranteed if we (1)
don't allow the module to unload if there are outstanding kobjects (we
may need a self destruct knob then) or (2) make sure the -release
functions are outside of the module (see, for example,
drivers/s390/s390_rdev.c).

(Gah, that stuff is always giving me headaches. Sorry if I'm not making
sense...)
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html