Re: [PATCH v3 2/3] nvme: Pass pointers, not dma addresses, to nvme_get/set_features()
On 09/16/2016 09:13 AM, Andy Lutomirski wrote: On Sep 16, 2016 1:41 AM, "Christoph Hellwig"wrote: On Thu, Sep 15, 2016 at 10:24:20PM -0700, Andy Lutomirski wrote: Any user I can imagine that needs a buffer at all will want to pass a pointer directly. There are no currently callers that use buffers, so this change is painless, and it will make it much easier to start using features that use buffers (e.g. APST). Signed-off-by: Andy Lutomirski --- Looks good mostly good, but a nitpick below: + /* + * Casting buffer to void* is safe here: __nvme_submit_sync_cmd knows + * that we're writing because it decodes the opcode. + */ + ret = __nvme_submit_sync_cmd(dev->admin_q, , , + (void *)buffer, buflen, 0, NVME_QID_ANY, 0, 0); Cant we just drop the const annotation to avoid these casts? Then we'd have nvme_set_feature() taking a non-const pointer, which would seem a little bit silly to me and might require the same cast somewhere else down the road. That'd still be a lot cleaner than the cast, which needs a comment to explain why it's supposedly safe. Just drop the const, imho. -- Jens Axboe
Re: [PATCH v3 2/3] nvme: Pass pointers, not dma addresses, to nvme_get/set_features()
On 09/16/2016 09:13 AM, Andy Lutomirski wrote: On Sep 16, 2016 1:41 AM, "Christoph Hellwig" wrote: On Thu, Sep 15, 2016 at 10:24:20PM -0700, Andy Lutomirski wrote: Any user I can imagine that needs a buffer at all will want to pass a pointer directly. There are no currently callers that use buffers, so this change is painless, and it will make it much easier to start using features that use buffers (e.g. APST). Signed-off-by: Andy Lutomirski --- Looks good mostly good, but a nitpick below: + /* + * Casting buffer to void* is safe here: __nvme_submit_sync_cmd knows + * that we're writing because it decodes the opcode. + */ + ret = __nvme_submit_sync_cmd(dev->admin_q, , , + (void *)buffer, buflen, 0, NVME_QID_ANY, 0, 0); Cant we just drop the const annotation to avoid these casts? Then we'd have nvme_set_feature() taking a non-const pointer, which would seem a little bit silly to me and might require the same cast somewhere else down the road. That'd still be a lot cleaner than the cast, which needs a comment to explain why it's supposedly safe. Just drop the const, imho. -- Jens Axboe
Re: [PATCH v3 2/3] nvme: Pass pointers, not dma addresses, to nvme_get/set_features()
On Sep 16, 2016 1:41 AM, "Christoph Hellwig"wrote: > > On Thu, Sep 15, 2016 at 10:24:20PM -0700, Andy Lutomirski wrote: > > Any user I can imagine that needs a buffer at all will want to pass > > a pointer directly. There are no currently callers that use > > buffers, so this change is painless, and it will make it much easier > > to start using features that use buffers (e.g. APST). > > > > Signed-off-by: Andy Lutomirski > > --- > > Looks good mostly good, but a nitpick below: > > > + /* > > + * Casting buffer to void* is safe here: __nvme_submit_sync_cmd knows > > + * that we're writing because it decodes the opcode. > > + */ > > + ret = __nvme_submit_sync_cmd(dev->admin_q, , , > > + (void *)buffer, buflen, 0, NVME_QID_ANY, 0, 0); > > Cant we just drop the const annotation to avoid these casts? > Then we'd have nvme_set_feature() taking a non-const pointer, which would seem a little bit silly to me and might require the same cast somewhere else down the road.
Re: [PATCH v3 2/3] nvme: Pass pointers, not dma addresses, to nvme_get/set_features()
On Sep 16, 2016 1:41 AM, "Christoph Hellwig" wrote: > > On Thu, Sep 15, 2016 at 10:24:20PM -0700, Andy Lutomirski wrote: > > Any user I can imagine that needs a buffer at all will want to pass > > a pointer directly. There are no currently callers that use > > buffers, so this change is painless, and it will make it much easier > > to start using features that use buffers (e.g. APST). > > > > Signed-off-by: Andy Lutomirski > > --- > > Looks good mostly good, but a nitpick below: > > > + /* > > + * Casting buffer to void* is safe here: __nvme_submit_sync_cmd knows > > + * that we're writing because it decodes the opcode. > > + */ > > + ret = __nvme_submit_sync_cmd(dev->admin_q, , , > > + (void *)buffer, buflen, 0, NVME_QID_ANY, 0, 0); > > Cant we just drop the const annotation to avoid these casts? > Then we'd have nvme_set_feature() taking a non-const pointer, which would seem a little bit silly to me and might require the same cast somewhere else down the road.
Re: [PATCH v3 2/3] nvme: Pass pointers, not dma addresses, to nvme_get/set_features()
On Thu, Sep 15, 2016 at 10:24:20PM -0700, Andy Lutomirski wrote: > Any user I can imagine that needs a buffer at all will want to pass > a pointer directly. There are no currently callers that use > buffers, so this change is painless, and it will make it much easier > to start using features that use buffers (e.g. APST). > > Signed-off-by: Andy Lutomirski> --- Looks good mostly good, but a nitpick below: > + /* > + * Casting buffer to void* is safe here: __nvme_submit_sync_cmd knows > + * that we're writing because it decodes the opcode. > + */ > + ret = __nvme_submit_sync_cmd(dev->admin_q, , , > + (void *)buffer, buflen, 0, NVME_QID_ANY, 0, 0); Cant we just drop the const annotation to avoid these casts?
Re: [PATCH v3 2/3] nvme: Pass pointers, not dma addresses, to nvme_get/set_features()
On Thu, Sep 15, 2016 at 10:24:20PM -0700, Andy Lutomirski wrote: > Any user I can imagine that needs a buffer at all will want to pass > a pointer directly. There are no currently callers that use > buffers, so this change is painless, and it will make it much easier > to start using features that use buffers (e.g. APST). > > Signed-off-by: Andy Lutomirski > --- Looks good mostly good, but a nitpick below: > + /* > + * Casting buffer to void* is safe here: __nvme_submit_sync_cmd knows > + * that we're writing because it decodes the opcode. > + */ > + ret = __nvme_submit_sync_cmd(dev->admin_q, , , > + (void *)buffer, buflen, 0, NVME_QID_ANY, 0, 0); Cant we just drop the const annotation to avoid these casts?