RE: [PATCH 02/15] hyperv: Add a function to detect hv_device

[KY Srinivasan]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@xxxxxxxxxxxxxxxxxxx]
> Sent: Thursday, December 1, 2016 10:48 PM
> To: KY Srinivasan <kys@xxxxxxxxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx; devel@xxxxxxxxxxxxxxxxxxxxxx;
> olaf@xxxxxxxxx; apw@xxxxxxxxxxxxx; vkuznets@xxxxxxxxxx;
> jasowang@xxxxxxxxxx; leann.ogasawara@xxxxxxxxxxxxx; Haiyang Zhang
> <haiyangz@xxxxxxxxxxxxx>
> Subject: Re: [PATCH 02/15] hyperv: Add a function to detect hv_device
> 
> On Fri, Dec 02, 2016 at 06:02:29AM +0000, KY Srinivasan wrote:
> >
> >
> > > -----Original Message-----
> > > From: Greg KH [mailto:gregkh@xxxxxxxxxxxxxxxxxxx]
> > > Sent: Thursday, December 1, 2016 12:36 PM
> > > To: KY Srinivasan <kys@xxxxxxxxxxxxx>
> > > Cc: linux-kernel@xxxxxxxxxxxxxxx; devel@xxxxxxxxxxxxxxxxxxxxxx;
> > > olaf@xxxxxxxxx; apw@xxxxxxxxxxxxx; vkuznets@xxxxxxxxxx;
> > > jasowang@xxxxxxxxxx; leann.ogasawara@xxxxxxxxxxxxx; Haiyang Zhang
> > > <haiyangz@xxxxxxxxxxxxx>
> > > Subject: Re: [PATCH 02/15] hyperv: Add a function to detect hv_device
> 
> Ugh, please fix your email client...
> 
> > >
> > > On Thu, Dec 01, 2016 at 09:28:39AM -0800, kys@xxxxxxxxxxxxxxxxxxxxxx
> > > wrote:
> > > > From: Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>
> > > >
> > > > Signed-off-by: Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>
> > > > Signed-off-by: K. Y. Srinivasan <kys@xxxxxxxxxxxxx>
> > > > ---
> > > >  drivers/hv/vmbus_drv.c |    6 ++++++
> > > >  include/linux/hyperv.h |    2 ++
> > > >  2 files changed, 8 insertions(+), 0 deletions(-)
> > > >
> > > > diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> > > > index 0276d2e..1730ac0 100644
> > > > --- a/drivers/hv/vmbus_drv.c
> > > > +++ b/drivers/hv/vmbus_drv.c
> > > > @@ -692,6 +692,12 @@ struct onmessage_work_context {
> > > >  	struct hv_message msg;
> > > >  };
> > > >
> > > > +bool device_is_hyperv(struct device *dev)
> > > > +{
> > > > +	return dev->release == vmbus_device_release;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(device_is_hyperv);
> > >
> > > Wait, eek, no!  That's NOT how you determine a device type, if you
> > > really even ever need to do that.
> > >
> > > Why are you needing this?  You should always "just know" what type of
> > > device a struct device * is, that's what we rely on in the driver model.
> > > Otherwise things get messy very very quickly.
> > >
> > > Sorry, I can't take this without a ton of justification, and even then,
> > > you need to do this correctly (and no, I'm not going to tell you how to
> > > do that as I don't like it being done...)
> > >
> > Greg,
> >
> > To support SR-IOV, netvsc registers for all netdev events. For netdev
> events related to the
> > VF interface, we need to do some special processing. And so, we need to
> determine
> > if the device that is generating the netdev event is Hyper-V device or not
> > (passed through the PCI pass through driver). Is this justification sufficient.
> > As you have observed, currently there is no user of this API and that is
> because netvsc
> > will be the user. To avoid cross-tree dependency, we wanted to get this
> functionality in first
> > before submitting the netvsc patch.
> 
> See, you do have some text for a changelog!
> 
> {grumble...}
> 
> No, I don't think this is a good justification, where are you going to
> put this "check" into the networking stack?  Your driver should only be
> binding to devices of this "type" anyway, so by that logic, it already
> "knows" that the device is of this type.
> 
> In other words, why do you need this and PCI or USB doesn't?  Why is
> hyperv "special"?

On Hyper-V, each VF interface (SR-IOV interface)
is paired with an instance of the 
synthetic interface that is managed by netvsc.
When the VF interface comes up, we
need to associate the VF instance with 
the corresponding netvsc instance. To do this
without modifying the VF drivers, netvsc registers 
for netdev events. In the netdev
event handler (in netvsc) currently I am 
doing the association based on the MAC
address - this code is currently committed 
upstream (see drivers/net/hyperv/netvsc_drv.c 
netvsc_register_vf()). Going forward, we want to 
base this association based on a sequence
number that the host publishes both for the 
VF as well as the corresponding
synthetic (netvsc) instance.

You are right, netvsc already knows that the 
devices it is managing belong to vmbus.
Since we are registering for netdev events, 
we will get notified for devices that may not
be vmbus devices and that is where this new 
API will be used. If the device is a vmbus device
we can extract the sequence number to implement the match.

Regards,

K. Y