On Wed, Nov 23, 2022 at 10:41:46PM +0000, Chaitanya Kulkarni wrote:
>
> > +/* Get lifetime information from device */
> > +static int virtblk_ioctl_lifetime(struct virtio_blk *vblk, unsigned long arg)
> > +{
> > + struct request_queue *q = vblk->disk->queue;
> > + struct request *req = NULL;
> > + struct virtblk_req *vbr;
> > + struct virtio_blk_lifetime lifetime;
> > + int ret;
> > +
> > + /* The virtio_blk_lifetime struct fields follow virtio spec.
> > + * There is no check/decode on values received from the device.
> > + * The data is sent as is to the user.
> > + */
>
> Odd comment style, why not :-
>
> /*
> * The virtio_blk_lifetime struct fields follow virtio spec.
> * There is no check/decode on values received from the device.
> * The data is sent as is to the user.
> */
most of virtio blk is like this. I don't really care much.
> > +
> > + /* This ioctl is allowed only if VIRTIO_BLK_F_LIFETIME
> > + * feature is negotiated.
> > + */
>
> above comment is redundant to the below code as following code is
> self explanatory.,..
i find it helpful personally - this point is important enough to
stress at cost of some duplication.
> > + if (!virtio_has_feature(vblk->vdev, VIRTIO_BLK_F_LIFETIME))
> > + return -ENOTTY;
> > +
> > + memset(&lifetime, 0, sizeof(lifetime));
> > +
> > + req = blk_mq_alloc_request(q, REQ_OP_DRV_IN, 0);
> > + if (IS_ERR(req))
> > + return PTR_ERR(req);
> > +
> > + /* Write the correct type */
>
> same here for above comment...
it's pretty harmless though
> > + vbr = blk_mq_rq_to_pdu(req);
> > + vbr->out_hdr.type = cpu_to_virtio32(vblk->vdev, VIRTIO_BLK_T_GET_LIFETIME);
> > +
> > + ret = blk_rq_map_kern(q, req, &lifetime, sizeof(lifetime), GFP_KERNEL);
> > + if (ret)
> > + goto out;
> > +
> > + blk_execute_rq(req, false);
> > +
> > + ret = virtblk_ioctl_result(blk_mq_rq_to_pdu(req));
> > + if (ret)
> > + goto out;
> > +
> > + /* Pass the data to the user */
> > + if (copy_to_user((void __user *)arg, &lifetime, sizeof(lifetime))) {
> > + ret = -EFAULT;
> > + goto out;
>
> there nothing here to "goto out" following is sufficient I guess :-
>
> if (copy_to_user((void __user *)arg, &lifetime, sizeof(lifetime)))
> ret = -EFAULT;
error handling with goto seems cleaner, easier to add code down
the road.
> > + }
> > +
> > +out:
> > + blk_mq_free_request(req);
> > + return ret;
> > +}
> > +
>
> [...]
>
> };
> >
> > +/* Get lifetime information struct for each request */
> > +struct virtio_blk_lifetime {
> > + /*
> > + * specifies the percentage of reserved blocks that are consumed.
> > + * optional values following virtio spec:
> > + * 0 - undefined
> > + * 1 - normal, < 80% of reserved blocks are consumed
> > + * 2 - warning, 80% of reserved blocks are consumed
> > + * 3 - urgent, 90% of reserved blocks are consumed
> > + */
> > + __le16 pre_eol_info;
> > + /*
> > + * this field refers to wear of SLC cells and is provided in increments of 10used,
> > + * and so on, thru to 11 meaning estimated lifetime exceeded. All values above 11
> > + * are reserved
> > + */
> > + __le16 device_lifetime_est_typ_a;
> > + /*
> > + * this field refers to wear of MLC cells and is provided with the same semantics as
> > + * device_lifetime_est_typ_a
> > + */
> > + __le16 device_lifetime_est_typ_b;
> > +};
> > +
>
> are you sure there won't be any new members ever be added in future ?
> to make it is futuresafe I'd add padding to this structure and keep
> the reserve bytes to the meaningful size...
>
> -ck
virtio has feature bits for this kind of thing, we don't do padding.
--
MST
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
