[RFC,net,1/3] virtio: re-negotiate features if probe fails and features are blocked
Commit Message
This patch exports a new virtio core function: virtio_block_feature.
The function should be called during a virtio driver probe.
If a virtio driver blocks features during probe and fails probe, virtio
core will reset the device, try to re-negotiate the new features and
probe again.
Signed-off-by: Alvaro Karsz <alvaro.karsz@solid-run.com>
---
drivers/virtio/virtio.c | 73 ++++++++++++++++++++++++++++++-----------
include/linux/virtio.h | 3 ++
2 files changed, 56 insertions(+), 20 deletions(-)
Comments
On Sun, Apr 30, 2023 at 04:15:16PM +0300, Alvaro Karsz wrote:
> This patch exports a new virtio core function: virtio_block_feature.
> The function should be called during a virtio driver probe.
>
> If a virtio driver blocks features during probe and fails probe, virtio
> core will reset the device, try to re-negotiate the new features and
> probe again.
>
> Signed-off-by: Alvaro Karsz <alvaro.karsz@solid-run.com>
> ---
> drivers/virtio/virtio.c | 73 ++++++++++++++++++++++++++++++-----------
> include/linux/virtio.h | 3 ++
> 2 files changed, 56 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index 3893dc29eb2..eaad5b6a7a9 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -167,6 +167,13 @@ void virtio_add_status(struct virtio_device *dev, unsigned int status)
> }
> EXPORT_SYMBOL_GPL(virtio_add_status);
>
> +void virtio_block_feature(struct virtio_device *dev, unsigned int f)
> +{
> + BUG_ON(f >= 64);
> + dev->blocked_features |= (1ULL << f);
> +}
> +EXPORT_SYMBOL_GPL(virtio_block_feature);
> +
Let's add documentation please. Also pls call it __virtio_block_feature
since it has to be used in a special way - specifically only during
probe.
> /* Do some validation, then set FEATURES_OK */
> static int virtio_features_ok(struct virtio_device *dev)
> {
> @@ -234,17 +241,13 @@ void virtio_reset_device(struct virtio_device *dev)
> }
> EXPORT_SYMBOL_GPL(virtio_reset_device);
>
> -static int virtio_dev_probe(struct device *_d)
> +static int virtio_negotiate_features(struct virtio_device *dev)
> {
> - int err, i;
> - struct virtio_device *dev = dev_to_virtio(_d);
> struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
> u64 device_features;
> u64 driver_features;
> u64 driver_features_legacy;
> -
> - /* We have a driver! */
> - virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER);
> + int i, ret;
>
> /* Figure out what features the device supports. */
> device_features = dev->config->get_features(dev);
> @@ -279,30 +282,61 @@ static int virtio_dev_probe(struct device *_d)
> if (device_features & (1ULL << i))
> __virtio_set_bit(dev, i);
>
> - err = dev->config->finalize_features(dev);
> - if (err)
> - goto err;
> + /* Remove blocked features */
> + dev->features &= ~dev->blocked_features;
> +
> + ret = dev->config->finalize_features(dev);
> + if (ret)
> + goto exit;
>
> if (drv->validate) {
> u64 features = dev->features;
>
> - err = drv->validate(dev);
> - if (err)
> - goto err;
> + ret = drv->validate(dev);
> + if (ret)
> + goto exit;
>
> /* Did validation change any features? Then write them again. */
> if (features != dev->features) {
> - err = dev->config->finalize_features(dev);
> - if (err)
> - goto err;
> + ret = dev->config->finalize_features(dev);
> + if (ret)
> + goto exit;
> }
> }
>
> - err = virtio_features_ok(dev);
> - if (err)
> - goto err;
> + ret = virtio_features_ok(dev);
> +exit:
> + return ret;
> +}
> +
> +static int virtio_dev_probe(struct device *_d)
> +{
> + int err;
> + struct virtio_device *dev = dev_to_virtio(_d);
> + struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
> + u64 blocked_features;
> + bool renegotiate = true;
> +
> + /* We have a driver! */
> + virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER);
> +
> + /* Store blocked features and attempt to negotiate features & probe.
> + * If the probe fails, we check if the driver has blocked any new features.
> + * If it has, we reset the device and try again with the new features.
> + */
> + while (renegotiate) {
> + blocked_features = dev->blocked_features;
> + err = virtio_negotiate_features(dev);
> + if (err)
> + break;
> +
> + err = drv->probe(dev);
there's no way to driver to clear blocked features, but
just in case, I'd add BUG_ON to check.
> + if (err && blocked_features != dev->blocked_features)
> + virtio_reset_device(dev);
> + else
> + renegotiate = false;
> + }
>
> - err = drv->probe(dev);
> if (err)
> goto err;
>
> @@ -319,7 +353,6 @@ static int virtio_dev_probe(struct device *_d)
> err:
> virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED);
> return err;
> -
> }
>
> static void virtio_dev_remove(struct device *_d)
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index b93238db94e..2de9b2d3ca4 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -109,6 +109,7 @@ int virtqueue_resize(struct virtqueue *vq, u32 num,
> * @vringh_config: configuration ops for host vrings.
> * @vqs: the list of virtqueues for this device.
> * @features: the features supported by both driver and device.
> + * @blocked_features: the features blocked by the driver that can't be negotiated.
> * @priv: private pointer for the driver's use.
> */
> struct virtio_device {
> @@ -124,6 +125,7 @@ struct virtio_device {
> const struct vringh_config_ops *vringh_config;
> struct list_head vqs;
> u64 features;
> + u64 blocked_features;
add comment here too, explain purpose and rules of use
> void *priv;
> };
>
> @@ -133,6 +135,7 @@ void virtio_add_status(struct virtio_device *dev, unsigned int status);
> int register_virtio_device(struct virtio_device *dev);
> void unregister_virtio_device(struct virtio_device *dev);
> bool is_virtio_device(struct device *dev);
> +void virtio_block_feature(struct virtio_device *dev, unsigned int f);
>
> void virtio_break_device(struct virtio_device *dev);
> void __virtio_unbreak_device(struct virtio_device *dev);
> --
> 2.34.1
> > +void virtio_block_feature(struct virtio_device *dev, unsigned int f)
> > +{
> > + BUG_ON(f >= 64);
> > + dev->blocked_features |= (1ULL << f);
> > +}
> > +EXPORT_SYMBOL_GPL(virtio_block_feature);
> > +
>
> Let's add documentation please. Also pls call it __virtio_block_feature
> since it has to be used in a special way - specifically only during
> probe.
>
Ok.
> > + /* Store blocked features and attempt to negotiate features & probe.
> > + * If the probe fails, we check if the driver has blocked any new features.
> > + * If it has, we reset the device and try again with the new features.
> > + */
> > + while (renegotiate) {
> > + blocked_features = dev->blocked_features;
> > + err = virtio_negotiate_features(dev);
> > + if (err)
> > + break;
> > +
> > + err = drv->probe(dev);
>
>
> there's no way to driver to clear blocked features, but
> just in case, I'd add BUG_ON to check.
>
Ok.
> > * @features: the features supported by both driver and device.
> > + * @blocked_features: the features blocked by the driver that can't be negotiated.
> > * @priv: private pointer for the driver's use.
> > */
> > struct virtio_device {
> > @@ -124,6 +125,7 @@ struct virtio_device {
> > const struct vringh_config_ops *vringh_config;
> > struct list_head vqs;
> > u64 features;
> > + u64 blocked_features;
>
> add comment here too, explain purpose and rules of use
>
Ok.
@@ -167,6 +167,13 @@ void virtio_add_status(struct virtio_device *dev, unsigned int status)
}
EXPORT_SYMBOL_GPL(virtio_add_status);
+void virtio_block_feature(struct virtio_device *dev, unsigned int f)
+{
+ BUG_ON(f >= 64);
+ dev->blocked_features |= (1ULL << f);
+}
+EXPORT_SYMBOL_GPL(virtio_block_feature);
+
/* Do some validation, then set FEATURES_OK */
static int virtio_features_ok(struct virtio_device *dev)
{
@@ -234,17 +241,13 @@ void virtio_reset_device(struct virtio_device *dev)
}
EXPORT_SYMBOL_GPL(virtio_reset_device);
-static int virtio_dev_probe(struct device *_d)
+static int virtio_negotiate_features(struct virtio_device *dev)
{
- int err, i;
- struct virtio_device *dev = dev_to_virtio(_d);
struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
u64 device_features;
u64 driver_features;
u64 driver_features_legacy;
-
- /* We have a driver! */
- virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER);
+ int i, ret;
/* Figure out what features the device supports. */
device_features = dev->config->get_features(dev);
@@ -279,30 +282,61 @@ static int virtio_dev_probe(struct device *_d)
if (device_features & (1ULL << i))
__virtio_set_bit(dev, i);
- err = dev->config->finalize_features(dev);
- if (err)
- goto err;
+ /* Remove blocked features */
+ dev->features &= ~dev->blocked_features;
+
+ ret = dev->config->finalize_features(dev);
+ if (ret)
+ goto exit;
if (drv->validate) {
u64 features = dev->features;
- err = drv->validate(dev);
- if (err)
- goto err;
+ ret = drv->validate(dev);
+ if (ret)
+ goto exit;
/* Did validation change any features? Then write them again. */
if (features != dev->features) {
- err = dev->config->finalize_features(dev);
- if (err)
- goto err;
+ ret = dev->config->finalize_features(dev);
+ if (ret)
+ goto exit;
}
}
- err = virtio_features_ok(dev);
- if (err)
- goto err;
+ ret = virtio_features_ok(dev);
+exit:
+ return ret;
+}
+
+static int virtio_dev_probe(struct device *_d)
+{
+ int err;
+ struct virtio_device *dev = dev_to_virtio(_d);
+ struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
+ u64 blocked_features;
+ bool renegotiate = true;
+
+ /* We have a driver! */
+ virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER);
+
+ /* Store blocked features and attempt to negotiate features & probe.
+ * If the probe fails, we check if the driver has blocked any new features.
+ * If it has, we reset the device and try again with the new features.
+ */
+ while (renegotiate) {
+ blocked_features = dev->blocked_features;
+ err = virtio_negotiate_features(dev);
+ if (err)
+ break;
+
+ err = drv->probe(dev);
+ if (err && blocked_features != dev->blocked_features)
+ virtio_reset_device(dev);
+ else
+ renegotiate = false;
+ }
- err = drv->probe(dev);
if (err)
goto err;
@@ -319,7 +353,6 @@ static int virtio_dev_probe(struct device *_d)
err:
virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED);
return err;
-
}
static void virtio_dev_remove(struct device *_d)
@@ -109,6 +109,7 @@ int virtqueue_resize(struct virtqueue *vq, u32 num,
* @vringh_config: configuration ops for host vrings.
* @vqs: the list of virtqueues for this device.
* @features: the features supported by both driver and device.
+ * @blocked_features: the features blocked by the driver that can't be negotiated.
* @priv: private pointer for the driver's use.
*/
struct virtio_device {
@@ -124,6 +125,7 @@ struct virtio_device {
const struct vringh_config_ops *vringh_config;
struct list_head vqs;
u64 features;
+ u64 blocked_features;
void *priv;
};
@@ -133,6 +135,7 @@ void virtio_add_status(struct virtio_device *dev, unsigned int status);
int register_virtio_device(struct virtio_device *dev);
void unregister_virtio_device(struct virtio_device *dev);
bool is_virtio_device(struct device *dev);
+void virtio_block_feature(struct virtio_device *dev, unsigned int f);
void virtio_break_device(struct virtio_device *dev);
void __virtio_unbreak_device(struct virtio_device *dev);