* [PATCH] virtio.h: correct comment for struct virtio_driver
From: Wang Sheng-Hui @ 2011-08-25 13:04 UTC (permalink / raw)
To: rusty, mst, virtualization, linux-kernel
The patch is against 3.0.
Signed-off-by: Wang Sheng-Hui <shhuiw@gmail.com>
---
include/linux/virtio.h | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 7108857..e0a1973 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -126,10 +126,10 @@ void unregister_virtio_device(struct virtio_device *dev);
* virtio_driver - operations for a virtio I/O driver
* @driver: underlying device driver (populate name and owner).
* @id_table: the ids serviced by this driver.
- * @feature_table: an array of feature numbers supported by this device.
+ * @feature_table: an array of feature numbers supported by this driver.
* @feature_table_size: number of entries in the feature table array.
* @probe: the function to call when a device is found. Returns 0 or -errno.
- * @remove: the function when a device is removed.
+ * @remove: the function to call when a device is removed.
* @config_changed: optional function to call when the device configuration
* changes; may be called in interrupt context.
*/
--
1.7.1
^ permalink raw reply related
* Re: [PATCH 086/117] Staging: hv: storvsc: Leverage the spinlock to manage ref_cnt
From: Greg KH @ 2011-08-25 2:45 UTC (permalink / raw)
To: KY Srinivasan
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
Haiyang Zhang
In-Reply-To: <6E21E5352C11B742B20C142EB499E048081B2A33@TK5EX14MBXC126.redmond.corp.microsoft.com>
On Wed, Aug 24, 2011 at 10:57:18PM +0000, KY Srinivasan wrote:
> > > Like other bus specific devices in the kernel (pci devices, virtio devices,),
> > > class specific vmbus devices - struct storvsc_device and struct netvsc_device
> > > embed a pointer to the underlying struct hv_device.
> >
> > And when you save that pointer, you ARE incrementing the reference count
> > properly, right? If not, you just caused a bug.
>
> Why do you say that. This assignment is done in the probe function where the
> driver core is invoking the driver specific probe function.
But if you save a pointer, you HAVE to increment the reference count.
> In the driver specific probe function, I allocate class specific
> device state and embed the bus specific device pointer. So, I would
> think the driver core is taking the appropriate reference count. What
> I am doing is exactly what other PCI and virtio drivers are doing. For
> instance, in virtio_blk.c , virtblk_probe() function, the
> virtio_device pointer is stashed away in the virtio_blk structure in
> exactly the same way I am doing. I suspect the assumption here is that
> if probe succeeded, then the remove() function would be called to let
> the driver cleanup the state.
Yes, but that's a bug, the pointer reference count should be
incremented.
Look at drivers/usb/usb-skeleton.c for a well-documented way to handle a
driver that works with a bus that has devices that could go away at any
point in time. It handles the reference count, and the locking
correctly, and has been audited by lots of people.
> > > Furthermore, a pointer to the class specific device structure is
> > > stashed in the struct hv_device (the ext pointer).
> > > This is identical what is done in the virtio blk device - look at the
> > > priv element in struct virtio_device.
> >
> > Yes, but the "base" structure of virtio_device does not contain a lock
> > that the users of that priv pointer are using to control access to data
> > _within_ the priv pointer, right?
>
> True. As I noted in an earlier email, the current hyper-v driver code has logic
> to deal with the race conditions I have described. It is just that the current
> implementation is completely bogus - look at for instance the function
> must_get_stor_device() in storvsc.c. This function is invoked in the channel
> callback code path to process messages coming from the host. I added this lock
> to close the window in getting the ext pointer. Clearly the lock protecting the
> ext pointer must be in a structure whose persistence is guaranteed and that is the reason
> I put the lock in the struct hv_device.
But no, that's not the way to do it, put it in the structure that has
the data you are trying to protect.
greg k-h
^ permalink raw reply
* Re: [PATCH 003/117] Staging: hv: Add struct hv_vmbus_device_id to mod_devicetable.h
From: Greg KH @ 2011-08-25 2:40 UTC (permalink / raw)
To: KY Srinivasan
Cc: devel@linuxdriverproject.org, Haiyang Zhang, gregkh@suse.de,
linux-kernel@vger.kernel.org, virtualization@lists.osdl.org
In-Reply-To: <6E21E5352C11B742B20C142EB499E048081B2AF2@TK5EX14MBXC126.redmond.corp.microsoft.com>
On Thu, Aug 25, 2011 at 02:27:56AM +0000, KY Srinivasan wrote:
> Since I don't have any (current) use for the driver_data pointer, I have gone ahead
> and cleaned up the first 74 patches without adding the driver_data.
> With the mushing of the patches you had proposed this is about
> a 60 patch series and addresses all the other comments you had in the first 74 patches.
> I hope I have gotten the "right" granularity now. If it is ok with you, I could send these
> out for your consideration.
Please do.
But if you do, do you mind if I add the driver_data pointer, so you can
blame me later if no one uses it? :)
> The only unresolved issue in the remaining patches (75 - 117) is the reference counting
> issue we have been debating. As I noted in my earlier emails on this topic, the reference
> counting has been there for a long time and I am reluctant get rid of that code without
> additional testing/analysis. So I want to propose the following options:
>
> 1) Keep the existing code and I will skip the patches that cleaned up the reference counting
>
> 2) Take the cleanup that I have implemented
>
> In either case, I would further test and analyze this code to see if (a) the race condition that is being
> addressed is valid and (b) if there is a different mechanism that could be used to deal with it. Given
> the gaping holes in the current implementation, my personal preference would be to go with the
> second option. Let me know what you want me to do here.
Ok, that sounds acceptable, but don't add the lock to the hv_driver, or
is that needed right now?
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 003/117] Staging: hv: Add struct hv_vmbus_device_id to mod_devicetable.h
From: Greg KH @ 2011-08-25 2:38 UTC (permalink / raw)
To: KY Srinivasan
Cc: devel@linuxdriverproject.org, Haiyang Zhang, gregkh@suse.de,
linux-kernel@vger.kernel.org, virtualization@lists.osdl.org
In-Reply-To: <6E21E5352C11B742B20C142EB499E048081B29C8@TK5EX14MBXC126.redmond.corp.microsoft.com>
On Wed, Aug 24, 2011 at 09:51:00PM +0000, KY Srinivasan wrote:
> > It would allow you, in your probe function, to do something different
> > depending on the guid that the probe function was matching on. So you
> > would not have to check the guid again to do that, just use the data
> > pointed in that void pointer and away you go.
> >
> > As an example, look at drivers/usb/class/cdc-acm.c, the acm_ids[]
> > variable which uses the driver_info field to contain a quirk for the
> > device.
>
> Ok; this makes sense. But I currently don't have any quirks to support!
That was just an example.
> The util driver is not even a driver in the true sense. I made it a driver and
> added the probe function just to support auto-loading with the vmbus ID space
> that I am trying to implement here - the probe function does nothing.
As you can tell, I'm trying to say that is wrong :)
the hv_util.c driver should have a pointer in each of the driver_info
that describes what it is, and then in the probe function, you use that
to hook up the correct things needed.
None of this hard-coded mess like you currently have.
Trust me, you want that pointer, put it in, and if you never use it,
I'll buy you a beer. But if I send you a patch using it, well...
> > > I looked at the usage of this in PCI and it appears to be for supporting
> > > dynamic IDs for existing drivers.
> >
> > No, that's exactly wrong. dynamic ids play havoc with this pointer,
> > making some drivers not be able to handle dynamic ids because they rely
> > on it for some driver-specific information to be passed in it, which
> > dynamic ids can not handle.
> >
> > Oh, have you remembered to turn off dynamic ids for these devices? Or
> > do you support them properly?
>
> I don't support dynamic IDs. What would I need to do to turn it off.
Ah, nevermind, that is something that busses add in their core if they
want it. I added it for PCI and USB, maybe I should move that to the
driver core so that others can get it automatically one of these days...
thanks,
greg k-h
^ permalink raw reply
* RE: [PATCH 003/117] Staging: hv: Add struct hv_vmbus_device_id to mod_devicetable.h
From: KY Srinivasan @ 2011-08-25 2:27 UTC (permalink / raw)
To: KY Srinivasan, Greg KH
Cc: devel@linuxdriverproject.org, Haiyang Zhang, gregkh@suse.de,
linux-kernel@vger.kernel.org, virtualization@lists.osdl.org
In-Reply-To: <6E21E5352C11B742B20C142EB499E048081B29C8@TK5EX14MBXC126.redmond.corp.microsoft.com>
> -----Original Message-----
> From: devel-bounces@linuxdriverproject.org [mailto:devel-
> bounces@linuxdriverproject.org] On Behalf Of KY Srinivasan
> Sent: Wednesday, August 24, 2011 5:51 PM
> To: Greg KH
> Cc: devel@linuxdriverproject.org; Haiyang Zhang; gregkh@suse.de; linux-
> kernel@vger.kernel.org; virtualization@lists.osdl.org
> Subject: RE: [PATCH 003/117] Staging: hv: Add struct hv_vmbus_device_id to
> mod_devicetable.h
>
>
>
> > -----Original Message-----
> > From: Greg KH [mailto:greg@kroah.com]
> > Sent: Wednesday, August 24, 2011 4:12 PM
> > To: KY Srinivasan
> > Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> > devel@linuxdriverproject.org; virtualization@lists.osdl.org; Haiyang Zhang
> > Subject: Re: [PATCH 003/117] Staging: hv: Add struct hv_vmbus_device_id to
> > mod_devicetable.h
> >
> > On Wed, Aug 24, 2011 at 04:46:11PM +0000, KY Srinivasan wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Greg KH [mailto:greg@kroah.com]
> > > > Sent: Tuesday, August 23, 2011 10:59 PM
> > > > To: KY Srinivasan
> > > > Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> > > > devel@linuxdriverproject.org; virtualization@lists.osdl.org; Haiyang Zhang
> > > > Subject: Re: [PATCH 003/117] Staging: hv: Add struct hv_vmbus_device_id
> to
> > > > mod_devicetable.h
> > > >
> > > > On Wed, Aug 24, 2011 at 12:44:38AM +0000, KY Srinivasan wrote:
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Greg KH [mailto:greg@kroah.com]
> > > > > > Sent: Tuesday, August 23, 2011 6:42 PM
> > > > > > To: KY Srinivasan
> > > > > > Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> > > > > > devel@linuxdriverproject.org; virtualization@lists.osdl.org; Haiyang
> Zhang
> > > > > > Subject: Re: [PATCH 003/117] Staging: hv: Add struct
> hv_vmbus_device_id
> > to
> > > > > > mod_devicetable.h
> > > > > >
> > > > > > On Fri, Jul 15, 2011 at 10:45:51AM -0700, K. Y. Srinivasan wrote:
> > > > > > > In preparation for implementing vmbus aliases for auto-loading
> > > > > > > Hyper-V drivers, define vmbus specific device ID.
> > > > > > >
> > > > > > > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > > > > > > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> > > > > > > ---
> > > > > > > include/linux/mod_devicetable.h | 7 +++++++
> > > > > > > 1 files changed, 7 insertions(+), 0 deletions(-)
> > > > > > >
> > > > > > > diff --git a/include/linux/mod_devicetable.h
> > > > > > b/include/linux/mod_devicetable.h
> > > > > > > index ae28e93..5a12377 100644
> > > > > > > --- a/include/linux/mod_devicetable.h
> > > > > > > +++ b/include/linux/mod_devicetable.h
> > > > > > > @@ -405,6 +405,13 @@ struct virtio_device_id {
> > > > > > > };
> > > > > > > #define VIRTIO_DEV_ANY_ID 0xffffffff
> > > > > > >
> > > > > > > +/*
> > > > > > > + * For Hyper-V devices we use the device guid as the id.
> > > > > > > + */
> > > > > > > +struct hv_vmbus_device_id {
> > > > > > > + __u8 guid[16];
> > > > > > > +};
> > > > > >
> > > > > > Why do you not need a driver_data pointer here? Are you sure you
> aren't
> > > > > > ever going to need it in the future? Hint, I think you will...
> > > > >
> > > > > I am not sure I am following you here; the guid is the device ID and it is
> > > > > guaranteed to remain the same. What is the driver _data pointer here
> > > > > you are referring to here. While some device id have the _data pointer,
> > > > > there are others that don't - for instance struct virtio_device_id. In
> > > > > our case, I am not sure how I would use this private pointer.
> > > >
> > > > You use it like all other drivers use it, only if needed.
> > >
> > > Fair enough; the point is I am not sure how I would use it.
> > >
> > > >
> > > > Hint, I think you need to use it in your hv_utils driver, it would
> > > > reduce the size of your code and simplify your logic.
> > >
> > > Could you expand on this. Currently the util driver handles a bunch
> > > services that have their own guids - and these have been included
> > > in the idtable. How would having the pointer simplify this code.
> >
> > It would allow you, in your probe function, to do something different
> > depending on the guid that the probe function was matching on. So you
> > would not have to check the guid again to do that, just use the data
> > pointed in that void pointer and away you go.
> >
> > As an example, look at drivers/usb/class/cdc-acm.c, the acm_ids[]
> > variable which uses the driver_info field to contain a quirk for the
> > device.
>
> Ok; this makes sense. But I currently don't have any quirks to support!
> The util driver is not even a driver in the true sense. I made it a driver and
> added the probe function just to support auto-loading with the vmbus ID space
> that I am trying to implement here - the probe function does nothing.
>
> So, if it is ok with you, I will not add driver_data pointer since I will not be
> using it.
>
> >
> > > I looked at the usage of this in PCI and it appears to be for supporting
> > > dynamic IDs for existing drivers.
> >
> > No, that's exactly wrong. dynamic ids play havoc with this pointer,
> > making some drivers not be able to handle dynamic ids because they rely
> > on it for some driver-specific information to be passed in it, which
> > dynamic ids can not handle.
> >
> > Oh, have you remembered to turn off dynamic ids for these devices? Or
> > do you support them properly?
>
> I don't support dynamic IDs. What would I need to do to turn it off.
Greg,
Since I don't have any (current) use for the driver_data pointer, I have gone ahead
and cleaned up the first 74 patches without adding the driver_data.
With the mushing of the patches you had proposed this is about
a 60 patch series and addresses all the other comments you had in the first 74 patches.
I hope I have gotten the "right" granularity now. If it is ok with you, I could send these
out for your consideration.
The only unresolved issue in the remaining patches (75 - 117) is the reference counting
issue we have been debating. As I noted in my earlier emails on this topic, the reference
counting has been there for a long time and I am reluctant get rid of that code without
additional testing/analysis. So I want to propose the following options:
1) Keep the existing code and I will skip the patches that cleaned up the reference counting
2) Take the cleanup that I have implemented
In either case, I would further test and analyze this code to see if (a) the race condition that is being
addressed is valid and (b) if there is a different mechanism that could be used to deal with it. Given
the gaping holes in the current implementation, my personal preference would be to go with the
second option. Let me know what you want me to do here.
Regards,
K. Y
> _______________________________________________
> devel mailing list
> devel@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
^ permalink raw reply
* RE: [PATCH 086/117] Staging: hv: storvsc: Leverage the spinlock to manage ref_cnt
From: KY Srinivasan @ 2011-08-24 22:57 UTC (permalink / raw)
To: Greg KH
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
Haiyang Zhang
In-Reply-To: <20110824201729.GB28667@kroah.com>
> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Wednesday, August 24, 2011 4:17 PM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; Haiyang Zhang
> Subject: Re: [PATCH 086/117] Staging: hv: storvsc: Leverage the spinlock to
> manage ref_cnt
>
> On Wed, Aug 24, 2011 at 04:25:18PM +0000, KY Srinivasan wrote:
> > > > > On Fri, Jul 15, 2011 at 10:47:14AM -0700, K. Y. Srinivasan wrote:
> > > > > > Now that we have a spin lock protecting access to the stor device
> pointer,
> > > > > > use it manage the reference count as well.
> > > > > >
> > > > > > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > > > > > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> > > > > > ---
> > > > > > drivers/staging/hv/hyperv_storage.h | 8 ++++----
> > > > > > drivers/staging/hv/storvsc.c | 10 +++++-----
> > > > > > 2 files changed, 9 insertions(+), 9 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/staging/hv/hyperv_storage.h
> > > > > b/drivers/staging/hv/hyperv_storage.h
> > > > > > index 53b65be..d946211 100644
> > > > > > --- a/drivers/staging/hv/hyperv_storage.h
> > > > > > +++ b/drivers/staging/hv/hyperv_storage.h
> > > > > > @@ -265,7 +265,7 @@ struct storvsc_device {
> > > > > > struct hv_device *device;
> > > > > >
> > > > > > /* 0 indicates the device is being destroyed */
> > > > > > - atomic_t ref_count;
> > > > > > + int ref_count;
> > > > >
> > > > > Is this really needed? Can't you rely on the reference count of the
> > > > > hv_device itself?
> > > >
> > > > We don't have a reference count on the hv_device
> > >
> > > Wait, why not? You shure better have a reference count on that device
> > > if you have a pointer to it, if not, you have a bug, and that needs to
> > > be fixed. Please reread Documentation/CodingStyle for details.
> >
> > Greg,
> >
> > I am confused here. The model we have is identical to other device/bus
> > abstractions in the kernel. For instance, neither struct pci_dev nor struct
> > virtio_device have an explicit reference count. However, they both embed
> > struct device which has the kobject structure embedded to manage
> > the object life cycle. This is exactly the model we have for the vmbus devices -
> > struct hv_device embeds struct device that embeds the struct kobject for
> > managing the lifecycle.
>
> Yes, that is correct.
>
> > Like other bus specific devices in the kernel (pci devices, virtio devices,),
> > class specific vmbus devices - struct storvsc_device and struct netvsc_device
> > embed a pointer to the underlying struct hv_device.
>
> And when you save that pointer, you ARE incrementing the reference count
> properly, right? If not, you just caused a bug.
Why do you say that. This assignment is done in the probe function where the
driver core is invoking the driver specific probe function. In the driver specific
probe function, I allocate class specific device state and embed the bus specific
device pointer. So, I would think the driver core is taking the appropriate reference
count. What I am doing is exactly what other PCI and virtio drivers are doing. For instance,
in virtio_blk.c , virtblk_probe() function, the virtio_device pointer is stashed away in the
virtio_blk structure in exactly the same way I am doing. I suspect the assumption here is that
if probe succeeded, then the remove() function would be called to let the driver cleanup the state.
>
> > Furthermore, a pointer to the class specific device structure is
> > stashed in the struct hv_device (the ext pointer).
> > This is identical what is done in the virtio blk device - look at the
> > priv element in struct virtio_device.
>
> Yes, but the "base" structure of virtio_device does not contain a lock
> that the users of that priv pointer are using to control access to data
> _within_ the priv pointer, right?
True. As I noted in an earlier email, the current hyper-v driver code has logic
to deal with the race conditions I have described. It is just that the current
implementation is completely bogus - look at for instance the function
must_get_stor_device() in storvsc.c. This function is invoked in the channel
callback code path to process messages coming from the host. I added this lock
to close the window in getting the ext pointer. Clearly the lock protecting the
ext pointer must be in a structure whose persistence is guaranteed and that is the reason
I put the lock in the struct hv_device.
>
> That's up to the users within the priv pointer.
>
> Now I see how you were trying to move the lock "deeper" as it seemed
> that everyone needed it, but you just moved the lock away from the thing
> that it was trying to protect, which might cause problems, and at the
> least, is confusing as heck because you are mixing two different layers
> here, in ways that should not be mixed.
>
> If you really need a lock to protect some private data within the priv
> pointer, then put it somewhere else,like in the priv pointer, as almost
> all other subsystems do.
What is being protected is the ext pointer that points to the class specific device state.
This state is freed when the driver is unloaded (in the remove function). The typical access
to the class specific device structure occurs given the pointer to bus specific device structure
struct hv_device. In the current code this transformation is done in functions such as
get_stor_device() and must_get_stor_device(). As you can see the logic in these function is
broken since there are multiple windows. The lock I introduced was to close the windows I saw in
these functions (similar functions exist on the netvsc side as well - get_inbound_net_device()
and get_outbound_net_device() in netvsc.c). I just tried to cleanup these functions with an easy to
understand logic to ensure the stability of the ext pointer.
Regards,
K. Y
^ permalink raw reply
* RE: [PATCH 003/117] Staging: hv: Add struct hv_vmbus_device_id to mod_devicetable.h
From: KY Srinivasan @ 2011-08-24 21:51 UTC (permalink / raw)
To: Greg KH
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
Haiyang Zhang
In-Reply-To: <20110824201144.GA28667@kroah.com>
> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Wednesday, August 24, 2011 4:12 PM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; Haiyang Zhang
> Subject: Re: [PATCH 003/117] Staging: hv: Add struct hv_vmbus_device_id to
> mod_devicetable.h
>
> On Wed, Aug 24, 2011 at 04:46:11PM +0000, KY Srinivasan wrote:
> >
> >
> > > -----Original Message-----
> > > From: Greg KH [mailto:greg@kroah.com]
> > > Sent: Tuesday, August 23, 2011 10:59 PM
> > > To: KY Srinivasan
> > > Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> > > devel@linuxdriverproject.org; virtualization@lists.osdl.org; Haiyang Zhang
> > > Subject: Re: [PATCH 003/117] Staging: hv: Add struct hv_vmbus_device_id to
> > > mod_devicetable.h
> > >
> > > On Wed, Aug 24, 2011 at 12:44:38AM +0000, KY Srinivasan wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Greg KH [mailto:greg@kroah.com]
> > > > > Sent: Tuesday, August 23, 2011 6:42 PM
> > > > > To: KY Srinivasan
> > > > > Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> > > > > devel@linuxdriverproject.org; virtualization@lists.osdl.org; Haiyang Zhang
> > > > > Subject: Re: [PATCH 003/117] Staging: hv: Add struct hv_vmbus_device_id
> to
> > > > > mod_devicetable.h
> > > > >
> > > > > On Fri, Jul 15, 2011 at 10:45:51AM -0700, K. Y. Srinivasan wrote:
> > > > > > In preparation for implementing vmbus aliases for auto-loading
> > > > > > Hyper-V drivers, define vmbus specific device ID.
> > > > > >
> > > > > > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > > > > > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> > > > > > ---
> > > > > > include/linux/mod_devicetable.h | 7 +++++++
> > > > > > 1 files changed, 7 insertions(+), 0 deletions(-)
> > > > > >
> > > > > > diff --git a/include/linux/mod_devicetable.h
> > > > > b/include/linux/mod_devicetable.h
> > > > > > index ae28e93..5a12377 100644
> > > > > > --- a/include/linux/mod_devicetable.h
> > > > > > +++ b/include/linux/mod_devicetable.h
> > > > > > @@ -405,6 +405,13 @@ struct virtio_device_id {
> > > > > > };
> > > > > > #define VIRTIO_DEV_ANY_ID 0xffffffff
> > > > > >
> > > > > > +/*
> > > > > > + * For Hyper-V devices we use the device guid as the id.
> > > > > > + */
> > > > > > +struct hv_vmbus_device_id {
> > > > > > + __u8 guid[16];
> > > > > > +};
> > > > >
> > > > > Why do you not need a driver_data pointer here? Are you sure you aren't
> > > > > ever going to need it in the future? Hint, I think you will...
> > > >
> > > > I am not sure I am following you here; the guid is the device ID and it is
> > > > guaranteed to remain the same. What is the driver _data pointer here
> > > > you are referring to here. While some device id have the _data pointer,
> > > > there are others that don't - for instance struct virtio_device_id. In
> > > > our case, I am not sure how I would use this private pointer.
> > >
> > > You use it like all other drivers use it, only if needed.
> >
> > Fair enough; the point is I am not sure how I would use it.
> >
> > >
> > > Hint, I think you need to use it in your hv_utils driver, it would
> > > reduce the size of your code and simplify your logic.
> >
> > Could you expand on this. Currently the util driver handles a bunch
> > services that have their own guids - and these have been included
> > in the idtable. How would having the pointer simplify this code.
>
> It would allow you, in your probe function, to do something different
> depending on the guid that the probe function was matching on. So you
> would not have to check the guid again to do that, just use the data
> pointed in that void pointer and away you go.
>
> As an example, look at drivers/usb/class/cdc-acm.c, the acm_ids[]
> variable which uses the driver_info field to contain a quirk for the
> device.
Ok; this makes sense. But I currently don't have any quirks to support!
The util driver is not even a driver in the true sense. I made it a driver and
added the probe function just to support auto-loading with the vmbus ID space
that I am trying to implement here - the probe function does nothing.
So, if it is ok with you, I will not add driver_data pointer since I will not be
using it.
>
> > I looked at the usage of this in PCI and it appears to be for supporting
> > dynamic IDs for existing drivers.
>
> No, that's exactly wrong. dynamic ids play havoc with this pointer,
> making some drivers not be able to handle dynamic ids because they rely
> on it for some driver-specific information to be passed in it, which
> dynamic ids can not handle.
>
> Oh, have you remembered to turn off dynamic ids for these devices? Or
> do you support them properly?
I don't support dynamic IDs. What would I need to do to turn it off.
Regards,
K. Y
^ permalink raw reply
* Re: [PATCH 086/117] Staging: hv: storvsc: Leverage the spinlock to manage ref_cnt
From: Greg KH @ 2011-08-24 20:17 UTC (permalink / raw)
To: KY Srinivasan
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
Haiyang Zhang
In-Reply-To: <6E21E5352C11B742B20C142EB499E048081B2789@TK5EX14MBXC126.redmond.corp.microsoft.com>
On Wed, Aug 24, 2011 at 04:25:18PM +0000, KY Srinivasan wrote:
> > > > On Fri, Jul 15, 2011 at 10:47:14AM -0700, K. Y. Srinivasan wrote:
> > > > > Now that we have a spin lock protecting access to the stor device pointer,
> > > > > use it manage the reference count as well.
> > > > >
> > > > > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > > > > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> > > > > ---
> > > > > drivers/staging/hv/hyperv_storage.h | 8 ++++----
> > > > > drivers/staging/hv/storvsc.c | 10 +++++-----
> > > > > 2 files changed, 9 insertions(+), 9 deletions(-)
> > > > >
> > > > > diff --git a/drivers/staging/hv/hyperv_storage.h
> > > > b/drivers/staging/hv/hyperv_storage.h
> > > > > index 53b65be..d946211 100644
> > > > > --- a/drivers/staging/hv/hyperv_storage.h
> > > > > +++ b/drivers/staging/hv/hyperv_storage.h
> > > > > @@ -265,7 +265,7 @@ struct storvsc_device {
> > > > > struct hv_device *device;
> > > > >
> > > > > /* 0 indicates the device is being destroyed */
> > > > > - atomic_t ref_count;
> > > > > + int ref_count;
> > > >
> > > > Is this really needed? Can't you rely on the reference count of the
> > > > hv_device itself?
> > >
> > > We don't have a reference count on the hv_device
> >
> > Wait, why not? You shure better have a reference count on that device
> > if you have a pointer to it, if not, you have a bug, and that needs to
> > be fixed. Please reread Documentation/CodingStyle for details.
>
> Greg,
>
> I am confused here. The model we have is identical to other device/bus
> abstractions in the kernel. For instance, neither struct pci_dev nor struct
> virtio_device have an explicit reference count. However, they both embed
> struct device which has the kobject structure embedded to manage
> the object life cycle. This is exactly the model we have for the vmbus devices -
> struct hv_device embeds struct device that embeds the struct kobject for
> managing the lifecycle.
Yes, that is correct.
> Like other bus specific devices in the kernel (pci devices, virtio devices,),
> class specific vmbus devices - struct storvsc_device and struct netvsc_device
> embed a pointer to the underlying struct hv_device.
And when you save that pointer, you ARE incrementing the reference count
properly, right? If not, you just caused a bug.
> Furthermore, a pointer to the class specific device structure is
> stashed in the struct hv_device (the ext pointer).
> This is identical what is done in the virtio blk device - look at the
> priv element in struct virtio_device.
Yes, but the "base" structure of virtio_device does not contain a lock
that the users of that priv pointer are using to control access to data
_within_ the priv pointer, right?
That's up to the users within the priv pointer.
Now I see how you were trying to move the lock "deeper" as it seemed
that everyone needed it, but you just moved the lock away from the thing
that it was trying to protect, which might cause problems, and at the
least, is confusing as heck because you are mixing two different layers
here, in ways that should not be mixed.
If you really need a lock to protect some private data within the priv
pointer, then put it somewhere else,like in the priv pointer, as almost
all other subsystems do.
greg k-h
^ permalink raw reply
* Re: [PATCH 003/117] Staging: hv: Add struct hv_vmbus_device_id to mod_devicetable.h
From: Greg KH @ 2011-08-24 20:11 UTC (permalink / raw)
To: KY Srinivasan
Cc: devel@linuxdriverproject.org, Haiyang Zhang, gregkh@suse.de,
linux-kernel@vger.kernel.org, virtualization@lists.osdl.org
In-Reply-To: <6E21E5352C11B742B20C142EB499E048081B27B9@TK5EX14MBXC126.redmond.corp.microsoft.com>
On Wed, Aug 24, 2011 at 04:46:11PM +0000, KY Srinivasan wrote:
>
>
> > -----Original Message-----
> > From: Greg KH [mailto:greg@kroah.com]
> > Sent: Tuesday, August 23, 2011 10:59 PM
> > To: KY Srinivasan
> > Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> > devel@linuxdriverproject.org; virtualization@lists.osdl.org; Haiyang Zhang
> > Subject: Re: [PATCH 003/117] Staging: hv: Add struct hv_vmbus_device_id to
> > mod_devicetable.h
> >
> > On Wed, Aug 24, 2011 at 12:44:38AM +0000, KY Srinivasan wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Greg KH [mailto:greg@kroah.com]
> > > > Sent: Tuesday, August 23, 2011 6:42 PM
> > > > To: KY Srinivasan
> > > > Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> > > > devel@linuxdriverproject.org; virtualization@lists.osdl.org; Haiyang Zhang
> > > > Subject: Re: [PATCH 003/117] Staging: hv: Add struct hv_vmbus_device_id to
> > > > mod_devicetable.h
> > > >
> > > > On Fri, Jul 15, 2011 at 10:45:51AM -0700, K. Y. Srinivasan wrote:
> > > > > In preparation for implementing vmbus aliases for auto-loading
> > > > > Hyper-V drivers, define vmbus specific device ID.
> > > > >
> > > > > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > > > > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> > > > > ---
> > > > > include/linux/mod_devicetable.h | 7 +++++++
> > > > > 1 files changed, 7 insertions(+), 0 deletions(-)
> > > > >
> > > > > diff --git a/include/linux/mod_devicetable.h
> > > > b/include/linux/mod_devicetable.h
> > > > > index ae28e93..5a12377 100644
> > > > > --- a/include/linux/mod_devicetable.h
> > > > > +++ b/include/linux/mod_devicetable.h
> > > > > @@ -405,6 +405,13 @@ struct virtio_device_id {
> > > > > };
> > > > > #define VIRTIO_DEV_ANY_ID 0xffffffff
> > > > >
> > > > > +/*
> > > > > + * For Hyper-V devices we use the device guid as the id.
> > > > > + */
> > > > > +struct hv_vmbus_device_id {
> > > > > + __u8 guid[16];
> > > > > +};
> > > >
> > > > Why do you not need a driver_data pointer here? Are you sure you aren't
> > > > ever going to need it in the future? Hint, I think you will...
> > >
> > > I am not sure I am following you here; the guid is the device ID and it is
> > > guaranteed to remain the same. What is the driver _data pointer here
> > > you are referring to here. While some device id have the _data pointer,
> > > there are others that don't - for instance struct virtio_device_id. In
> > > our case, I am not sure how I would use this private pointer.
> >
> > You use it like all other drivers use it, only if needed.
>
> Fair enough; the point is I am not sure how I would use it.
>
> >
> > Hint, I think you need to use it in your hv_utils driver, it would
> > reduce the size of your code and simplify your logic.
>
> Could you expand on this. Currently the util driver handles a bunch
> services that have their own guids - and these have been included
> in the idtable. How would having the pointer simplify this code.
It would allow you, in your probe function, to do something different
depending on the guid that the probe function was matching on. So you
would not have to check the guid again to do that, just use the data
pointed in that void pointer and away you go.
As an example, look at drivers/usb/class/cdc-acm.c, the acm_ids[]
variable which uses the driver_info field to contain a quirk for the
device.
> I looked at the usage of this in PCI and it appears to be for supporting
> dynamic IDs for existing drivers.
No, that's exactly wrong. dynamic ids play havoc with this pointer,
making some drivers not be able to handle dynamic ids because they rely
on it for some driver-specific information to be passed in it, which
dynamic ids can not handle.
Oh, have you remembered to turn off dynamic ids for these devices? Or
do you support them properly?
greg k-h
^ permalink raw reply
* RE: [PATCH 003/117] Staging: hv: Add struct hv_vmbus_device_id to mod_devicetable.h
From: KY Srinivasan @ 2011-08-24 16:46 UTC (permalink / raw)
To: Greg KH
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
Haiyang Zhang
In-Reply-To: <20110824025913.GC30779@kroah.com>
> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Tuesday, August 23, 2011 10:59 PM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; Haiyang Zhang
> Subject: Re: [PATCH 003/117] Staging: hv: Add struct hv_vmbus_device_id to
> mod_devicetable.h
>
> On Wed, Aug 24, 2011 at 12:44:38AM +0000, KY Srinivasan wrote:
> >
> >
> > > -----Original Message-----
> > > From: Greg KH [mailto:greg@kroah.com]
> > > Sent: Tuesday, August 23, 2011 6:42 PM
> > > To: KY Srinivasan
> > > Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> > > devel@linuxdriverproject.org; virtualization@lists.osdl.org; Haiyang Zhang
> > > Subject: Re: [PATCH 003/117] Staging: hv: Add struct hv_vmbus_device_id to
> > > mod_devicetable.h
> > >
> > > On Fri, Jul 15, 2011 at 10:45:51AM -0700, K. Y. Srinivasan wrote:
> > > > In preparation for implementing vmbus aliases for auto-loading
> > > > Hyper-V drivers, define vmbus specific device ID.
> > > >
> > > > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > > > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> > > > ---
> > > > include/linux/mod_devicetable.h | 7 +++++++
> > > > 1 files changed, 7 insertions(+), 0 deletions(-)
> > > >
> > > > diff --git a/include/linux/mod_devicetable.h
> > > b/include/linux/mod_devicetable.h
> > > > index ae28e93..5a12377 100644
> > > > --- a/include/linux/mod_devicetable.h
> > > > +++ b/include/linux/mod_devicetable.h
> > > > @@ -405,6 +405,13 @@ struct virtio_device_id {
> > > > };
> > > > #define VIRTIO_DEV_ANY_ID 0xffffffff
> > > >
> > > > +/*
> > > > + * For Hyper-V devices we use the device guid as the id.
> > > > + */
> > > > +struct hv_vmbus_device_id {
> > > > + __u8 guid[16];
> > > > +};
> > >
> > > Why do you not need a driver_data pointer here? Are you sure you aren't
> > > ever going to need it in the future? Hint, I think you will...
> >
> > I am not sure I am following you here; the guid is the device ID and it is
> > guaranteed to remain the same. What is the driver _data pointer here
> > you are referring to here. While some device id have the _data pointer,
> > there are others that don't - for instance struct virtio_device_id. In
> > our case, I am not sure how I would use this private pointer.
>
> You use it like all other drivers use it, only if needed.
Fair enough; the point is I am not sure how I would use it.
>
> Hint, I think you need to use it in your hv_utils driver, it would
> reduce the size of your code and simplify your logic.
Could you expand on this. Currently the util driver handles a bunch
services that have their own guids - and these have been included
in the idtable. How would having the pointer simplify this code.
I looked at the usage of this in PCI and it appears to be for supporting
dynamic IDs for existing drivers. I am not sure if this is applicable to the util
driver. First, I am not sure if there will be additional services being
packaged into util driver and secondly, even if I implement this scheme
of being able to dynamically add new IDs, we still would need to add the
code to the driver to handle the new service. In fact not a whole lot of
code is shared amongst the services that are currently packaged as the
util driver.
Regards,
K. Y
^ permalink raw reply
* RE: [PATCH 086/117] Staging: hv: storvsc: Leverage the spinlock to manage ref_cnt
From: KY Srinivasan @ 2011-08-24 16:25 UTC (permalink / raw)
To: Greg KH
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
Haiyang Zhang
In-Reply-To: <20110824025737.GA30779@kroah.com>
> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Tuesday, August 23, 2011 10:58 PM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; Haiyang Zhang
> Subject: Re: [PATCH 086/117] Staging: hv: storvsc: Leverage the spinlock to
> manage ref_cnt
>
> On Wed, Aug 24, 2011 at 12:58:36AM +0000, KY Srinivasan wrote:
> >
> >
> > > -----Original Message-----
> > > From: Greg KH [mailto:greg@kroah.com]
> > > Sent: Tuesday, August 23, 2011 7:10 PM
> > > To: KY Srinivasan
> > > Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> > > devel@linuxdriverproject.org; virtualization@lists.osdl.org; Haiyang Zhang
> > > Subject: Re: [PATCH 086/117] Staging: hv: storvsc: Leverage the spinlock to
> > > manage ref_cnt
> > >
> > > On Fri, Jul 15, 2011 at 10:47:14AM -0700, K. Y. Srinivasan wrote:
> > > > Now that we have a spin lock protecting access to the stor device pointer,
> > > > use it manage the reference count as well.
> > > >
> > > > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > > > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> > > > ---
> > > > drivers/staging/hv/hyperv_storage.h | 8 ++++----
> > > > drivers/staging/hv/storvsc.c | 10 +++++-----
> > > > 2 files changed, 9 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/drivers/staging/hv/hyperv_storage.h
> > > b/drivers/staging/hv/hyperv_storage.h
> > > > index 53b65be..d946211 100644
> > > > --- a/drivers/staging/hv/hyperv_storage.h
> > > > +++ b/drivers/staging/hv/hyperv_storage.h
> > > > @@ -265,7 +265,7 @@ struct storvsc_device {
> > > > struct hv_device *device;
> > > >
> > > > /* 0 indicates the device is being destroyed */
> > > > - atomic_t ref_count;
> > > > + int ref_count;
> > >
> > > Is this really needed? Can't you rely on the reference count of the
> > > hv_device itself?
> >
> > We don't have a reference count on the hv_device
>
> Wait, why not? You shure better have a reference count on that device
> if you have a pointer to it, if not, you have a bug, and that needs to
> be fixed. Please reread Documentation/CodingStyle for details.
Greg,
I am confused here. The model we have is identical to other device/bus
abstractions in the kernel. For instance, neither struct pci_dev nor struct
virtio_device have an explicit reference count. However, they both embed
struct device which has the kobject structure embedded to manage
the object life cycle. This is exactly the model we have for the vmbus devices -
struct hv_device embeds struct device that embeds the struct kobject for
managing the lifecycle.
Like other bus specific devices in the kernel (pci devices, virtio devices,),
class specific vmbus devices - struct storvsc_device and struct netvsc_device
embed a pointer to the underlying struct hv_device. Furthermore, a pointer to
the class specific device structure is stashed in the struct hv_device (the ext pointer).
This is identical what is done in the virtio blk device - look at the priv element in struct virtio_device.
As I noted in a different email, I did not introduce this reference counting, I just fixed some gaping
holes in the logic. The reason, I fixed the logic and kept the reference counting is because I can
see cases where we could end up de-referencing a NULL pointer from the interrupt side that is
trying to deliver a vmbus packet to a device that is being destroyed.
Regards,
K. Y
>
> > and this count is taken to deal with racing unloads and incoming
> > traffic on the channel from the host.
>
> Is this something that all other storage drivers have to do? If not,
> then you shouldn't be doing that as well.
>
> greg k-h
^ permalink raw reply
* RE: [PATCH 081/117] Staging: hv: vmbus: Introduce a lock to protect the ext field in hv_device
From: KY Srinivasan @ 2011-08-24 14:29 UTC (permalink / raw)
To: Greg KH
Cc: devel@linuxdriverproject.org, Haiyang Zhang, gregkh@suse.de,
linux-kernel@vger.kernel.org, virtualization@lists.osdl.org
In-Reply-To: <20110824025827.GB30779@kroah.com>
> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Tuesday, August 23, 2011 10:58 PM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; Haiyang Zhang
> Subject: Re: [PATCH 081/117] Staging: hv: vmbus: Introduce a lock to protect the
> ext field in hv_device
>
> On Wed, Aug 24, 2011 at 12:55:12AM +0000, KY Srinivasan wrote:
> >
> >
> > > -----Original Message-----
> > > From: Greg KH [mailto:greg@kroah.com]
> > > Sent: Tuesday, August 23, 2011 7:08 PM
> > > To: KY Srinivasan
> > > Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> > > devel@linuxdriverproject.org; virtualization@lists.osdl.org; Haiyang Zhang
> > > Subject: Re: [PATCH 081/117] Staging: hv: vmbus: Introduce a lock to protect
> the
> > > ext field in hv_device
> > >
> > > On Fri, Jul 15, 2011 at 10:47:09AM -0700, K. Y. Srinivasan wrote:
> > > > The current mechanism for handling references in broken.
> > > > Introduce a lock to protect the ext field in hv_device.
> > >
> > > Why would that lock ever be needed? How can things change to this
> > > pointer in different ways like you are thinking it could? Doesn't the
> > > reference counting in the device itself handle this properly?
> >
> > This is to deal with a potential race condition between the driver being
> > unloaded and incoming traffic from the VMBUS side. The ext pointer is
> > device specific (either pointing to a storage or a network device) and what
> > we are protecting is the pointer being set to NULL from the unload side when
> > we might have a racing access from the interrupt side (on the incoming vmbus
> > traffic).
>
> I still don't think this is needed at all, the drivers should not have
> to worry about this. Something is wrong with the design if it is, as no
> other bus needs something like this, right?
This notion of reference counting has been in this code ever since I have been
working on this code; it just that I fixed some obvious holes/race conditions in the
logic to manage the reference counting. Firstly, this is not mandated by the vmbus and is
used to support the protocol between the guest and host for storage, networking etc.
At least in the case of both block and networking, the client side driver code (running in the guest)
Initiates book keeping messages to the host that is not known to upper level code in Linux. So for
Instance, even though SCSI layer knows exactly what I/O's are pending and takes care of reference
counting the structures, it does not know about messages that have been sent by the lower level
storvsc driver code. If a racing unload were to happen, we could potentially dereference a NULL
ext pointer from the incoming packet processing context. The cases where this can happen are even
more prevalent on the networking side.
Regards,
K. Y
^ permalink raw reply
* RE: [PATCH 019/117] Staging: hv: vmbus: Cleanup vmbus_uevent() code
From: KY Srinivasan @ 2011-08-24 14:12 UTC (permalink / raw)
To: Greg KH
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
Haiyang Zhang
In-Reply-To: <20110824030025.GD30779@kroah.com>
> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Tuesday, August 23, 2011 11:00 PM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; Haiyang Zhang
> Subject: Re: [PATCH 019/117] Staging: hv: vmbus: Cleanup vmbus_uevent() code
>
> On Wed, Aug 24, 2011 at 12:38:09AM +0000, KY Srinivasan wrote:
> >
> >
> > > -----Original Message-----
> > > From: Greg KH [mailto:greg@kroah.com]
> > > Sent: Tuesday, August 23, 2011 6:50 PM
> > > To: KY Srinivasan
> > > Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> > > devel@linuxdriverproject.org; virtualization@lists.osdl.org; Haiyang Zhang
> > > Subject: Re: [PATCH 019/117] Staging: hv: vmbus: Cleanup vmbus_uevent()
> code
> > >
> > > On Fri, Jul 15, 2011 at 10:46:07AM -0700, K. Y. Srinivasan wrote:
> > > > Now generate appropriate uevent based on the modalias string. As part of
> this,
> > > > cleanup the existing uevent code.
> > >
> > > Note, you just change the user api here, did you have tools that relied
> > > on the old format? If so, they just broke :(
> >
> > Prior to this, I don't think autoloading worked the way it should for these
> > modules.
>
> It didn't? How did the mouse driver get autoloaded then, through the
> pci/dmi tables?
You are right, prior to this all drivers were loaded using pci/dmi signatures.
Regards,
K. Y
^ permalink raw reply
* Re: [PATCH 0000/0117] Staging: hv: Driver cleanup
From: Greg KH @ 2011-08-24 3:08 UTC (permalink / raw)
To: KY Srinivasan
Cc: devel@linuxdriverproject.org, gregkh@suse.de,
linux-kernel@vger.kernel.org, virtualization@lists.osdl.org
In-Reply-To: <6E21E5352C11B742B20C142EB499E048081B23B1@TK5EX14MBXC126.redmond.corp.microsoft.com>
On Wed, Aug 24, 2011 at 01:01:26AM +0000, KY Srinivasan wrote:
> Thank you for your comments. I will try to re-submit these patches as
> soon as I possibly can. If I were to get these patches re-spun and
> sent to you in the next couple of days, can I still make this current
> merge window?
You ever sit down and write an email, pour out your frustrations and
anger in a long, detailed rant? Smoke coming off of your keyboard as
the keys start melting in the fury of the frustration driving your rage?
And then, afterward, re-reading it, realizing that you now felt at peace
with yourself, and such a rant was only for your benefit, the recipient,
while surely knowing better than to have caused the response in the
first place, really didn't deserve the flame in the end? So you then
delete the email before sending it, and then write a simple and concise
response instead, following the "three sentences"[1] rule?
Here's that response:
Read Documentation/development-process/ for how Linux kernel
development works and the merge windows happen.
Please never ask me again.
Ever.
greg k-h
[1] http://three.sentenc.es/
^ permalink raw reply
* Re: [PATCH 019/117] Staging: hv: vmbus: Cleanup vmbus_uevent() code
From: Greg KH @ 2011-08-24 3:00 UTC (permalink / raw)
To: KY Srinivasan
Cc: devel@linuxdriverproject.org, Haiyang Zhang, gregkh@suse.de,
linux-kernel@vger.kernel.org, virtualization@lists.osdl.org
In-Reply-To: <6E21E5352C11B742B20C142EB499E048081B2356@TK5EX14MBXC126.redmond.corp.microsoft.com>
On Wed, Aug 24, 2011 at 12:38:09AM +0000, KY Srinivasan wrote:
>
>
> > -----Original Message-----
> > From: Greg KH [mailto:greg@kroah.com]
> > Sent: Tuesday, August 23, 2011 6:50 PM
> > To: KY Srinivasan
> > Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> > devel@linuxdriverproject.org; virtualization@lists.osdl.org; Haiyang Zhang
> > Subject: Re: [PATCH 019/117] Staging: hv: vmbus: Cleanup vmbus_uevent() code
> >
> > On Fri, Jul 15, 2011 at 10:46:07AM -0700, K. Y. Srinivasan wrote:
> > > Now generate appropriate uevent based on the modalias string. As part of this,
> > > cleanup the existing uevent code.
> >
> > Note, you just change the user api here, did you have tools that relied
> > on the old format? If so, they just broke :(
>
> Prior to this, I don't think autoloading worked the way it should for these
> modules.
It didn't? How did the mouse driver get autoloaded then, through the
pci/dmi tables?
> > > + for (i = 0; i < (sizeof(struct hv_vmbus_device_id) * 2); i += 2)
> > > + sprintf(&alias_name[i], "%02x", dev->dev_type.b[i/2]);
> >
> > Don't we have a type for printing out a uuid already?
>
> I did not see one; could you point me to the right place.
> >
> > And what's with the jumping by 2 yet dividing? What am I missing here?
>
> Each byte of the uuid is represented by 2 bytes in the string; thus the magic with 2.
Ok, wierd, but it makes sense, thanks.
greg k-h
^ permalink raw reply
* Re: [PATCH 003/117] Staging: hv: Add struct hv_vmbus_device_id to mod_devicetable.h
From: Greg KH @ 2011-08-24 2:59 UTC (permalink / raw)
To: KY Srinivasan
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
Haiyang Zhang
In-Reply-To: <6E21E5352C11B742B20C142EB499E048081B236E@TK5EX14MBXC126.redmond.corp.microsoft.com>
On Wed, Aug 24, 2011 at 12:44:38AM +0000, KY Srinivasan wrote:
>
>
> > -----Original Message-----
> > From: Greg KH [mailto:greg@kroah.com]
> > Sent: Tuesday, August 23, 2011 6:42 PM
> > To: KY Srinivasan
> > Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> > devel@linuxdriverproject.org; virtualization@lists.osdl.org; Haiyang Zhang
> > Subject: Re: [PATCH 003/117] Staging: hv: Add struct hv_vmbus_device_id to
> > mod_devicetable.h
> >
> > On Fri, Jul 15, 2011 at 10:45:51AM -0700, K. Y. Srinivasan wrote:
> > > In preparation for implementing vmbus aliases for auto-loading
> > > Hyper-V drivers, define vmbus specific device ID.
> > >
> > > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> > > ---
> > > include/linux/mod_devicetable.h | 7 +++++++
> > > 1 files changed, 7 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/include/linux/mod_devicetable.h
> > b/include/linux/mod_devicetable.h
> > > index ae28e93..5a12377 100644
> > > --- a/include/linux/mod_devicetable.h
> > > +++ b/include/linux/mod_devicetable.h
> > > @@ -405,6 +405,13 @@ struct virtio_device_id {
> > > };
> > > #define VIRTIO_DEV_ANY_ID 0xffffffff
> > >
> > > +/*
> > > + * For Hyper-V devices we use the device guid as the id.
> > > + */
> > > +struct hv_vmbus_device_id {
> > > + __u8 guid[16];
> > > +};
> >
> > Why do you not need a driver_data pointer here? Are you sure you aren't
> > ever going to need it in the future? Hint, I think you will...
>
> I am not sure I am following you here; the guid is the device ID and it is
> guaranteed to remain the same. What is the driver _data pointer here
> you are referring to here. While some device id have the _data pointer,
> there are others that don't - for instance struct virtio_device_id. In
> our case, I am not sure how I would use this private pointer.
You use it like all other drivers use it, only if needed.
Hint, I think you need to use it in your hv_utils driver, it would
reduce the size of your code and simplify your logic.
And yes, not all bus types use it, but most do, for good reason.
greg k-h
^ permalink raw reply
* Re: [PATCH 081/117] Staging: hv: vmbus: Introduce a lock to protect the ext field in hv_device
From: Greg KH @ 2011-08-24 2:58 UTC (permalink / raw)
To: KY Srinivasan
Cc: devel@linuxdriverproject.org, Haiyang Zhang, gregkh@suse.de,
linux-kernel@vger.kernel.org, virtualization@lists.osdl.org
In-Reply-To: <6E21E5352C11B742B20C142EB499E048081B238B@TK5EX14MBXC126.redmond.corp.microsoft.com>
On Wed, Aug 24, 2011 at 12:55:12AM +0000, KY Srinivasan wrote:
>
>
> > -----Original Message-----
> > From: Greg KH [mailto:greg@kroah.com]
> > Sent: Tuesday, August 23, 2011 7:08 PM
> > To: KY Srinivasan
> > Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> > devel@linuxdriverproject.org; virtualization@lists.osdl.org; Haiyang Zhang
> > Subject: Re: [PATCH 081/117] Staging: hv: vmbus: Introduce a lock to protect the
> > ext field in hv_device
> >
> > On Fri, Jul 15, 2011 at 10:47:09AM -0700, K. Y. Srinivasan wrote:
> > > The current mechanism for handling references in broken.
> > > Introduce a lock to protect the ext field in hv_device.
> >
> > Why would that lock ever be needed? How can things change to this
> > pointer in different ways like you are thinking it could? Doesn't the
> > reference counting in the device itself handle this properly?
>
> This is to deal with a potential race condition between the driver being
> unloaded and incoming traffic from the VMBUS side. The ext pointer is
> device specific (either pointing to a storage or a network device) and what
> we are protecting is the pointer being set to NULL from the unload side when
> we might have a racing access from the interrupt side (on the incoming vmbus
> traffic).
I still don't think this is needed at all, the drivers should not have
to worry about this. Something is wrong with the design if it is, as no
other bus needs something like this, right?
greg k-h
^ permalink raw reply
* Re: [PATCH 086/117] Staging: hv: storvsc: Leverage the spinlock to manage ref_cnt
From: Greg KH @ 2011-08-24 2:57 UTC (permalink / raw)
To: KY Srinivasan
Cc: devel@linuxdriverproject.org, Haiyang Zhang, gregkh@suse.de,
linux-kernel@vger.kernel.org, virtualization@lists.osdl.org
In-Reply-To: <6E21E5352C11B742B20C142EB499E048081B239E@TK5EX14MBXC126.redmond.corp.microsoft.com>
On Wed, Aug 24, 2011 at 12:58:36AM +0000, KY Srinivasan wrote:
>
>
> > -----Original Message-----
> > From: Greg KH [mailto:greg@kroah.com]
> > Sent: Tuesday, August 23, 2011 7:10 PM
> > To: KY Srinivasan
> > Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> > devel@linuxdriverproject.org; virtualization@lists.osdl.org; Haiyang Zhang
> > Subject: Re: [PATCH 086/117] Staging: hv: storvsc: Leverage the spinlock to
> > manage ref_cnt
> >
> > On Fri, Jul 15, 2011 at 10:47:14AM -0700, K. Y. Srinivasan wrote:
> > > Now that we have a spin lock protecting access to the stor device pointer,
> > > use it manage the reference count as well.
> > >
> > > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> > > ---
> > > drivers/staging/hv/hyperv_storage.h | 8 ++++----
> > > drivers/staging/hv/storvsc.c | 10 +++++-----
> > > 2 files changed, 9 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/staging/hv/hyperv_storage.h
> > b/drivers/staging/hv/hyperv_storage.h
> > > index 53b65be..d946211 100644
> > > --- a/drivers/staging/hv/hyperv_storage.h
> > > +++ b/drivers/staging/hv/hyperv_storage.h
> > > @@ -265,7 +265,7 @@ struct storvsc_device {
> > > struct hv_device *device;
> > >
> > > /* 0 indicates the device is being destroyed */
> > > - atomic_t ref_count;
> > > + int ref_count;
> >
> > Is this really needed? Can't you rely on the reference count of the
> > hv_device itself?
>
> We don't have a reference count on the hv_device
Wait, why not? You shure better have a reference count on that device
if you have a pointer to it, if not, you have a bug, and that needs to
be fixed. Please reread Documentation/CodingStyle for details.
> and this count is taken to deal with racing unloads and incoming
> traffic on the channel from the host.
Is this something that all other storage drivers have to do? If not,
then you shouldn't be doing that as well.
greg k-h
^ permalink raw reply
* RE: [PATCH 4/8] Staging: hv: vmbus: Fix checkpatch warnings
From: KY Srinivasan @ 2011-08-24 1:10 UTC (permalink / raw)
To: Greg KH
Cc: devel@linuxdriverproject.org, Haiyang Zhang, gregkh@suse.de,
linux-kernel@vger.kernel.org, virtualization@lists.osdl.org
In-Reply-To: <20110823231657.GC9955@kroah.com>
> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Tuesday, August 23, 2011 7:17 PM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; Haiyang Zhang
> Subject: Re: [PATCH 4/8] Staging: hv: vmbus: Fix checkpatch warnings
>
> On Tue, Jul 19, 2011 at 11:44:21AM -0700, K. Y. Srinivasan wrote:
> > Fix checkpatch warnings in hv.c
> >
> > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> > ---
> > drivers/staging/hv/hv.c | 4 ++--
> > 1 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/staging/hv/hv.c b/drivers/staging/hv/hv.c
> > index e733173..14e6315 100644
> > --- a/drivers/staging/hv/hv.c
> > +++ b/drivers/staging/hv/hv.c
> > @@ -111,7 +111,7 @@ static u64 do_hypercall(u64 control, void *input, void
> *output)
> > u64 hv_status = 0;
> > u64 input_address = (input) ? virt_to_phys(input) : 0;
> > u64 output_address = (output) ? virt_to_phys(output) : 0;
> > - volatile void *hypercall_page = hv_context.hypercall_page;
> > + void *hypercall_page = hv_context.hypercall_page;
>
> Are you sure? This was just someone being foolish? No other reason
> someone tried to use volatile here?
I cannot see any reason why this needs to be volatile.
Regards,
K. Y
>
> greg k-h
^ permalink raw reply
* RE: [PATCH 0000/0117] Staging: hv: Driver cleanup
From: KY Srinivasan @ 2011-08-24 1:01 UTC (permalink / raw)
To: Greg KH
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org
In-Reply-To: <20110823231120.GP9641@kroah.com>
> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Tuesday, August 23, 2011 7:11 PM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org
> Subject: Re: [PATCH 0000/0117] Staging: hv: Driver cleanup
>
> On Fri, Jul 15, 2011 at 10:47:04AM -0700, K. Y. Srinivasan wrote:
> > Further cleanup of the hv drivers. Back in June I had sent two patch
> > sets to address these issues. I have addressed the comments I got from
> > the community on my earlier patches here:
> >
> > 1) Implement code for autoloading the vmbus drivers without using PCI
> or DMI
> > signatures. I have implemented this based on Greg's feedback on my
> earlier
> > implementation.
> >
> > 2) Cleanup error handling across the board and use standard Linux error
> codes.
> >
> > 3) General cleanup
> >
> > 4) Some bug fixes.
> >
> > 5) Cleanup the reference counting mess for both stor and net devices.
> >
> > 6) Handle all block devices using the storvsc driver. I have modified
> > the implementation here based on Christoph's feedback on my earlier
> > implementation.
> >
> > 7) Accomodate some host side scsi emulation bugs.
> >
> > 8) In case of scsi errors off-line the device.
>
> Care to respin this patch series based on my comments?
Greg,
Thank you for your comments. I will try to re-submit these patches as soon as I
possibly can. If I were to get these patches re-spun and sent to you in the next couple of
days, can I still make this current merge window?
Regards,
K. Y
>
> thanks,
>
> greg k-h
^ permalink raw reply
* RE: [PATCH 086/117] Staging: hv: storvsc: Leverage the spinlock to manage ref_cnt
From: KY Srinivasan @ 2011-08-24 0:58 UTC (permalink / raw)
To: Greg KH
Cc: devel@linuxdriverproject.org, Haiyang Zhang, gregkh@suse.de,
linux-kernel@vger.kernel.org, virtualization@lists.osdl.org
In-Reply-To: <20110823231028.GO9641@kroah.com>
> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Tuesday, August 23, 2011 7:10 PM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; Haiyang Zhang
> Subject: Re: [PATCH 086/117] Staging: hv: storvsc: Leverage the spinlock to
> manage ref_cnt
>
> On Fri, Jul 15, 2011 at 10:47:14AM -0700, K. Y. Srinivasan wrote:
> > Now that we have a spin lock protecting access to the stor device pointer,
> > use it manage the reference count as well.
> >
> > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> > ---
> > drivers/staging/hv/hyperv_storage.h | 8 ++++----
> > drivers/staging/hv/storvsc.c | 10 +++++-----
> > 2 files changed, 9 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/staging/hv/hyperv_storage.h
> b/drivers/staging/hv/hyperv_storage.h
> > index 53b65be..d946211 100644
> > --- a/drivers/staging/hv/hyperv_storage.h
> > +++ b/drivers/staging/hv/hyperv_storage.h
> > @@ -265,7 +265,7 @@ struct storvsc_device {
> > struct hv_device *device;
> >
> > /* 0 indicates the device is being destroyed */
> > - atomic_t ref_count;
> > + int ref_count;
>
> Is this really needed? Can't you rely on the reference count of the
> hv_device itself?
We don't have a reference count on the hv_device and this count is taken
to deal with racing unloads and incoming traffic on the channel from the
host.
Regards,
K. Y
^ permalink raw reply
* RE: [PATCH 081/117] Staging: hv: vmbus: Introduce a lock to protect the ext field in hv_device
From: KY Srinivasan @ 2011-08-24 0:55 UTC (permalink / raw)
To: Greg KH
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
Haiyang Zhang
In-Reply-To: <20110823230816.GN9641@kroah.com>
> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Tuesday, August 23, 2011 7:08 PM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; Haiyang Zhang
> Subject: Re: [PATCH 081/117] Staging: hv: vmbus: Introduce a lock to protect the
> ext field in hv_device
>
> On Fri, Jul 15, 2011 at 10:47:09AM -0700, K. Y. Srinivasan wrote:
> > The current mechanism for handling references in broken.
> > Introduce a lock to protect the ext field in hv_device.
>
> Why would that lock ever be needed? How can things change to this
> pointer in different ways like you are thinking it could? Doesn't the
> reference counting in the device itself handle this properly?
This is to deal with a potential race condition between the driver being
unloaded and incoming traffic from the VMBUS side. The ext pointer is
device specific (either pointing to a storage or a network device) and what
we are protecting is the pointer being set to NULL from the unload side when
we might have a racing access from the interrupt side (on the incoming vmbus
traffic).
Regards,
K. Y
^ permalink raw reply
* RE: [PATCH 003/117] Staging: hv: Add struct hv_vmbus_device_id to mod_devicetable.h
From: KY Srinivasan @ 2011-08-24 0:44 UTC (permalink / raw)
To: Greg KH
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
Haiyang Zhang
In-Reply-To: <20110823224157.GA9641@kroah.com>
> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Tuesday, August 23, 2011 6:42 PM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; Haiyang Zhang
> Subject: Re: [PATCH 003/117] Staging: hv: Add struct hv_vmbus_device_id to
> mod_devicetable.h
>
> On Fri, Jul 15, 2011 at 10:45:51AM -0700, K. Y. Srinivasan wrote:
> > In preparation for implementing vmbus aliases for auto-loading
> > Hyper-V drivers, define vmbus specific device ID.
> >
> > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> > ---
> > include/linux/mod_devicetable.h | 7 +++++++
> > 1 files changed, 7 insertions(+), 0 deletions(-)
> >
> > diff --git a/include/linux/mod_devicetable.h
> b/include/linux/mod_devicetable.h
> > index ae28e93..5a12377 100644
> > --- a/include/linux/mod_devicetable.h
> > +++ b/include/linux/mod_devicetable.h
> > @@ -405,6 +405,13 @@ struct virtio_device_id {
> > };
> > #define VIRTIO_DEV_ANY_ID 0xffffffff
> >
> > +/*
> > + * For Hyper-V devices we use the device guid as the id.
> > + */
> > +struct hv_vmbus_device_id {
> > + __u8 guid[16];
> > +};
>
> Why do you not need a driver_data pointer here? Are you sure you aren't
> ever going to need it in the future? Hint, I think you will...
I am not sure I am following you here; the guid is the device ID and it is
guaranteed to remain the same. What is the driver _data pointer here
you are referring to here. While some device id have the _data pointer,
there are others that don't - for instance struct virtio_device_id. In
our case, I am not sure how I would use this private pointer.
Regards,
K. Y
>
> greg k-h
^ permalink raw reply
* RE: [PATCH 019/117] Staging: hv: vmbus: Cleanup vmbus_uevent() code
From: KY Srinivasan @ 2011-08-24 0:38 UTC (permalink / raw)
To: Greg KH
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
Haiyang Zhang
In-Reply-To: <20110823224932.GF9641@kroah.com>
> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Tuesday, August 23, 2011 6:50 PM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; Haiyang Zhang
> Subject: Re: [PATCH 019/117] Staging: hv: vmbus: Cleanup vmbus_uevent() code
>
> On Fri, Jul 15, 2011 at 10:46:07AM -0700, K. Y. Srinivasan wrote:
> > Now generate appropriate uevent based on the modalias string. As part of this,
> > cleanup the existing uevent code.
>
> Note, you just change the user api here, did you have tools that relied
> on the old format? If so, they just broke :(
Prior to this, I don't think autoloading worked the way it should for these
modules.
>
> > + for (i = 0; i < (sizeof(struct hv_vmbus_device_id) * 2); i += 2)
> > + sprintf(&alias_name[i], "%02x", dev->dev_type.b[i/2]);
>
> Don't we have a type for printing out a uuid already?
I did not see one; could you point me to the right place.
>
> And what's with the jumping by 2 yet dividing? What am I missing here?
Each byte of the uuid is represented by 2 bytes in the string; thus the magic with 2.
Regards,
K. Y
^ permalink raw reply
* Re: [PATCH 0/8] Staging: hv: vmbus: Driver cleanup
From: Greg KH @ 2011-08-23 23:18 UTC (permalink / raw)
To: K. Y. Srinivasan; +Cc: devel, gregkh, linux-kernel, virtualization
In-Reply-To: <1313446210-10611-1-git-send-email-kys@microsoft.com>
On Mon, Aug 15, 2011 at 03:10:10PM -0700, K. Y. Srinivasan wrote:
> Further cleanup of the vmbus driver:
>
> 1) Cleanup the interrupt handler by inlining some code.
>
> 2) Ensure message handling is performed on the same CPU that
> takes the vmbus interrupt.
>
> 3) Check for events before messages (from the host).
>
> 4) Disable auto eoi for the vmbus interrupt since Linux will eoi the
> interrupt anyway.
>
> 5) Some general cleanup.
As I didn't apply your other changes, care to respin all of your
outstanding hv patches against the next linux-next release and resend
them? I tried picking some of the other patches, but they started not
applying easily so it would be easier for everyone to just resend them
after you fix them up.
thanks,
greg k-h
^ 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