On Mon, Jan 19, 2015 at 05:45:54PM +0000, Pawel Moll wrote:
> On Thu, 2015-01-15 at 19:12 +0000, Michael S. Tsirkin wrote:
> > > > > > > > > +static struct device_attribute vm_dev_attr_version =
> > > > > > > > > + __ATTR(version, S_IRUGO, vm_dev_attr_version_show, NULL);
> > > > > > > > > +
> > > > > > > > > static int virtio_mmio_probe(struct platform_device *pdev)
> > > > > > > > > {
> > > > > > > > > struct virtio_mmio_device *vm_dev;
> > > > > > > >
> > > > > > > > We already expose feature bits - this one really necessary?
> > > > > > >
> > > > > > > Necessary? Of course not, just a debugging feature, really, to see what
> > > > > > > version of control registers are available. Useful - I strongly believe
> > > > > > > so.
> > > > > >
> > > > > > Yes but the point is the same info is already available
> > > > > > in core: just look at feature bit 31.
> > > > > > If you think it's important enough to expose in a decoded
> > > > > > way, let's add this in core?
> > > > >
> > > > > How do you mean "in core"? It's a mmio-specific value. Content of the
> > > > > VIRTIO_MMIO_VERSION control register.
> > > >
> > > > Yes but if driver loaded, then revision is always in sync
> > > > with the feature bit.
> > >
> > > Well, not quite: as of now I've got legacy block device driver happily
> > > working on top of compliant (so v2 in MMIO speech) MMIO device - the
> > > transport if completely transparent here.
> >
> > Spec says explicitly it's an illegal configuration.
>
> What part of the spec exactly? The closest I can think of are 2.2.3, 6.2
> and 6.3. Both legacy and spec-complaint MMIO transports will satisfy all
> requirements from those paragraphs, whether VIRTIO_F_VERSION_1 is
> negotiated or not.
The parts that say VIRTIO_F_VERSION_1 MUST be set.
I'll look up the chapter and verse later if you like.
> Make no mistake - I don't know why would anyone wanted to do this kind
> of mishmash (other than for testing purposes), but from the MMIO
> transport point of view, it's not a problem.
>
> Does the check in finalize_features() solves the problem? If I
> understand correctly it should, so there's nothing more to be done,
> either in the MMIO spec or in the driver.
It does but it must be in driver since there are
transitional drivers so we can't just fail finalize_features
in core.
> > > > If driver failed to attach, attribute is not there
> > > > so can't be used for debugging.
> > >
> > > Interesting point on its own, haven't thought of that. This is an issue
> > > with platform devices, no standard set of attributes, always available.
> > > Will have a look at this.
> > >
> > > > > > Absolutely. So what happens if you drop these code lines?
> > > > > > There's no driver registered for this ID, so it's just ignored.
> > > > > > Seems like what spec is asking for, no?
> > > > >
> > > > > Not to me, no. There will be a vm_dev registered with an _illegal_ ID.
> > > >
> > > > Yes - there will be a device, but no driver will drive it.
> > >
> > > A device with an *illegal* ID.
> >
> > So? The "device" is just a bunch of allocated memory. There's no driver
> > and no one touches is, which is what the spec requires.
>
> So what exactly is wrong with the "if (id == 0) ignore" case handling? I
> bet a compare-to-zero and a branch in two places takes less memory than
> the allocated struct virtio_device. And certainly takes less cycles (not
> that this matters at all :-). And it's pretty well documented in the
> spec.
> > > > > > > > @@ -496,7 +531,8 @@ static int virtio_mmio_remove(struct platform_device *pdev)
> > > > > > > > > {
> > > > > > > > > struct virtio_mmio_device *vm_dev = platform_get_drvdata(pdev);
> > > > > > > > >
> > > > > > > > > - unregister_virtio_device(&vm_dev->vdev);
> > > > > > > > > + if (vm_dev)
> > > > > > > > > + unregister_virtio_device(&vm_dev->vdev);
> > > > > > > > >
> > > > > > > >
> > > > > > > > Will remove ever be called if probe fails?
> > > > > > >
> > > > > > > No.
> > > > > >
> > > > > > Then this if isn't necessary: vm_dev is always set.
> > > > >
> > > > > Not (in the current code) if ID is 0.
> > > >
> > > > So just return -ENODEV, then probe will be considered
> > > > failed and remove won't be called.
> > >
> > > "4.2.2.2 Driver Requirements: MMIO Device Register Layout
> > >
> > > The driver MUST ignore a device with DeviceID 0x0, but MUST NOT report any error."
> > >
> > > Returning -ENODEV *reports* an error.
> >
> > Reports to whom? Do you refer to http://xkcd.com/838/ ?
> > ENODEV is not "reported".
>
> No, you're right - -ENODEV doesn't print out "probe of xxx failed with
> error yyy" message, I was wrong. I'm pretty sure I tried it before (I
> was hoping to find an error which would block probing in a silent way)
> and I saw something, but it was probably the pr_debug() level message.
>
> > It's just a function return value,
> > it tells kernel not to call remove.
> > Users don't know: module probe succeeds, core will not print any errors, user is not
> > queried.
> >
> > What happens with your patch? Driver is attached to
> > device (check where does driver attribute points to!),
> > but doesn't do anything.
> >
> > Looks like if you just keep going, you'll achieve
> > the same result.
>
> Yes, returning -ENODEV when ID == 0 seems perfectly fine for me.
>
> > > > Or - better - just register the device, it's harmless
> > > > as no driver will try to attach to it, and there
> > > > won't be any need to special-case it.
> > >
> > > Really, you may want to refresh your memory. We've been there. This *is*
> > > a special case. Intentionally.
> >
> > Hmm I found https://issues.oasis-open.org/browse/VIRTIO-7
> > but the resolution there is merely asking that no driver
> > matches ID 0. Seems OK.
> > The MMIO changes were all made as part of
> > https://issues.oasis-open.org/browse/VIRTIO-44
> > Anyway, my memory is irrelevant, we need to document motivation
> > for kernel code changes for future readers.
>
> As a refresher:
>
> https://lists.oasis-open.org/archives/virtio/201307/msg00035.html
>
> and in particular:
>
> > On Wed, 2013-07-31 at 15:04 +0100, Michael S. Tsirkin wrote:
> > > I meant using standard bus specific things where we are describing bus
> > > specific behaviour.
> > > In this case, I think you really want a "no device" flag for
> > > virtio-mmio which originally lacked device presence detection.
> > > I think for this purpose, it is best to to:
> > > 1. declare device ID 0 (or e.g. FFFF) as reserved and illegal for devices,
> > > explicitly in core spec
> > > 2. in the mmio appendix, say that if ID 0 (or FFFF) is detected, this
> > > means device not present
> > > I think this is the cleaner approach than saying there's
> > > a dummy "null device" in particular because
> > > 1. there won't be dummy devices on the bus, in sysfs etc
> > > 2. down the road you will be able to support hotplug.
Okay, I see, thanks!
> It seems that you personally weren't happy about "dummy devices on the
> bus"...
>
> > It seems that dropping this
> > chunk satisfies the spec, so if not true, let's add a comment in code
> > that explains why.
> >
> > Assume you stick in device with ID 0.
> > kernel probes the device, and then sees there is no driver
> > and leaves it alone.
> > Seems perfect, matches the spec.
> >
> > Do you have a config that's not handled well here?
>
> The end effect will be the same, I agree. I simply disagree with your
> claim that it matches the spec and I see no point of discussing this
> subject further. I'm going to keep the special case with -ENODEV in the
> driver and apply other changes you suggested. If you want you can NAK
> the updated patch and we'll find someone to resolve the argument.
>
> Pawel
Can you please also add a comment?
E.g.
/*
* MMIO uses ID 0 to mean device not present. Avoid probing
* the device further: it's sure to fail anyway.
*/
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
