Amit Shah
2015-Jan-20 10:40 UTC
[PATCH v3 04/16] virtio/console: verify device has config space
On (Wed) 14 Jan 2015 [19:27:35], Michael S. Tsirkin wrote:> Some devices might not implement config space access > (e.g. remoteproc used not to - before 3.9). > virtio/console needs config space access so make it > fail gracefully if not there.Do we know any such devices? Wondering what prompted this patch. If it's just theoretical, I'd rather let it be like this, and pull this in when there's a device that doesn't have config space. Also, just the console functionality (i.e. F_MULTIPORT is unset) is available w/o config space access. In fact, getting this patch in would mean remoteproc wouldn't even run in its pre-config days...> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c > index de03df9..26afb56 100644 > --- a/drivers/char/virtio_console.c > +++ b/drivers/char/virtio_console.c > @@ -1986,6 +1986,12 @@ static int virtcons_probe(struct virtio_device *vdev) > bool multiport; > bool early = early_put_chars != NULL; > > + if (!vdev->config->get) { > + dev_err(&vdev->dev, "%s failure: config access disabled\n", > + __func__); > + return -EINVAL; > + } > +Amit
Michael S. Tsirkin
2015-Jan-20 11:09 UTC
[PATCH v3 04/16] virtio/console: verify device has config space
On Tue, Jan 20, 2015 at 04:10:40PM +0530, Amit Shah wrote:> On (Wed) 14 Jan 2015 [19:27:35], Michael S. Tsirkin wrote: > > Some devices might not implement config space access > > (e.g. remoteproc used not to - before 3.9). > > virtio/console needs config space access so make it > > fail gracefully if not there. > > Do we know any such devices? Wondering what prompted this patch. If > it's just theoretical, I'd rather let it be like this, and pull this > in when there's a device that doesn't have config space.Yes, with virtio 1.0 config space can be in a separate BAR now. If that's not enabled by BIOS (e.g. out of space), we won't have config space.> Also, just the console functionality (i.e. F_MULTIPORT is unset) is > available w/o config space access.Supporting this by gracefully disabling F_MULTIPORT would require getting this info from driver before features are finalized. Alternatively, check F_MULTIPORT and only fail if set? Let me know, I'll cook up a patch.> In fact, getting this patch in > would mean remoteproc wouldn't even run in its pre-config days...It seems to have get callback unconditionally now - or did I miss something?> > diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c > > index de03df9..26afb56 100644 > > --- a/drivers/char/virtio_console.c > > +++ b/drivers/char/virtio_console.c > > @@ -1986,6 +1986,12 @@ static int virtcons_probe(struct virtio_device *vdev) > > bool multiport; > > bool early = early_put_chars != NULL; > > > > + if (!vdev->config->get) { > > + dev_err(&vdev->dev, "%s failure: config access disabled\n", > > + __func__); > > + return -EINVAL; > > + } > > + > > > Amit
Amit Shah
2015-Jan-21 06:14 UTC
[PATCH v3 04/16] virtio/console: verify device has config space
On (Tue) 20 Jan 2015 [13:09:55], Michael S. Tsirkin wrote:> On Tue, Jan 20, 2015 at 04:10:40PM +0530, Amit Shah wrote: > > On (Wed) 14 Jan 2015 [19:27:35], Michael S. Tsirkin wrote: > > > Some devices might not implement config space access > > > (e.g. remoteproc used not to - before 3.9). > > > virtio/console needs config space access so make it > > > fail gracefully if not there. > > > > Do we know any such devices? Wondering what prompted this patch. If > > it's just theoretical, I'd rather let it be like this, and pull this > > in when there's a device that doesn't have config space. > > Yes, with virtio 1.0 config space can be in a separate BAR now. If > that's not enabled by BIOS (e.g. out of space), we won't have config > space.I'm still not sure whether we should pull in this patch before actually seeing a failure. You do have a dev_err which tells why the probe failed, so it's an acceptable compromise I suppose.> > Also, just the console functionality (i.e. F_MULTIPORT is unset) is > > available w/o config space access. > > Supporting this by gracefully disabling F_MULTIPORT > would require getting this info from driver before > features are finalized. > Alternatively, check F_MULTIPORT and only fail if set? > Let me know, I'll cook up a patch.Yes, failing only if F_MULTIPORT is set is a better option (if we have to fail).> > In fact, getting this patch in > > would mean remoteproc wouldn't even run in its pre-config days... > > It seems to have get callback unconditionally now - or did I miss > something?What I meant was remoteproc doesn't depend on the config space, only uses the console functionality. If remoteproc devices didn't expose a config space, this patch would cause it to lose its console functionality for no apparent reason. Amit
Seemingly Similar Threads
- [PATCH v3 04/16] virtio/console: verify device has config space
- [PATCH v3 04/16] virtio/console: verify device has config space
- [PATCH v3 04/16] virtio/console: verify device has config space
- [PATCH v3 04/16] virtio/console: verify device has config space
- [PATCH v3 04/16] virtio/console: verify device has config space