lkml.org 
[lkml]   [2020]   [Jul]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v3] firewire: Remove function callback casts
    Hi,

    Anyone has had time to review this patch? Any comments on this?

    On Sat, May 30, 2020 at 11:08:39AM +0200, Oscar Carter wrote:
    > In 1394 OHCI specification, Isochronous Receive DMA context has several
    > modes. One of mode is 'BufferFill' and Linux FireWire stack uses it to
    > receive isochronous packets for multiple isochronous channel as
    > FW_ISO_CONTEXT_RECEIVE_MULTICHANNEL.
    >
    > The mode is not used by in-kernel driver, while it's available for
    > userspace. The character device driver in firewire-core includes
    > cast of function callback for the mode since the type of callback
    > function is different from the other modes. The case is inconvenient
    > to effort of Control Flow Integrity builds due to
    > -Wcast-function-type warning.
    >
    > This commit removes the cast. A static helper function is newly added
    > to initialize isochronous context for the mode. The helper function
    > arranges isochronous context to assign specific callback function
    > after call of existent kernel API. It's noticeable that the number of
    > isochronous channel, speed, and the size of header are not required for
    > the mode. The helper function is used for the mode by character device
    > driver instead of direct call of existent kernel API.
    >
    > The same goal can be achieved (in the ioctl_create_iso_context function)
    > without this helper function as follows:
    > - Call the fw_iso_context_create function passing NULL to the callback
    > parameter.
    > - Then setting the context->callback.sc or context->callback.mc
    > variables based on the a->type value.
    >
    > However using the helper function created in this patch makes code more
    > clear and declarative. This way avoid the call to a function with one
    > purpose to achieved another one.
    >
    > Co-developed-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
    > Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
    > Co-developed-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
    > Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
    > Signed-off-by: Oscar Carter <oscar.carter@gmx.com>
    > ---
    > Hi,
    >
    > this is another proposal to achieved the goal of remove function callback
    > cast start by me with the first [1] and second [2] versions, and followed
    > by the work of Takashi Sakamoto with his first [3] and second [4] versions,
    > and the code of Stefan Richter [5].
    >
    > The purpose of this third version is to put together all the work done
    > until now following the comments of all reviewed patches.
    >
    > I've added the "Co-developed-by" and "Signed-off-by" tags to give credit to
    > Takashi Sakamoto and Stefan Richter if there are no objections.
    >
    > Changelog v1->v2
    > -Set explicity to NULL the "ctx->callback.sc" variable and return an error
    > code in "fw_iso_context_create" function if both callback parameters are
    > NULL as Lev R. Oshvang suggested.
    > -Modify the commit changelog accordingly.
    >
    > Changelog v2->v3
    > -Put togeher all the work done in different patches by different authors.
    > -Modify the previous work following the comments in the reviewed patches.
    >
    > [1] https://lore.kernel.org/lkml/20200516173934.31527-1-oscar.carter@gmx.com/
    > [2] https://lore.kernel.org/lkml/20200519173425.4724-1-oscar.carter@gmx.com/
    > [3] https://lore.kernel.org/lkml/20200520064726.31838-1-o-takashi@sakamocchi.jp/
    > [4] https://lore.kernel.org/lkml/20200524132048.243223-1-o-takashi@sakamocchi.jp/
    > [5] https://lore.kernel.org/lkml/20200525015532.0055f9df@kant/
    >
    > drivers/firewire/core-cdev.c | 32 ++++++++++++++++++++++++++------
    > include/linux/firewire.h | 11 +++++++----
    > 2 files changed, 33 insertions(+), 10 deletions(-)
    >
    > diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
    > index 6e291d8f3a27..f7212331f245 100644
    > --- a/drivers/firewire/core-cdev.c
    > +++ b/drivers/firewire/core-cdev.c
    > @@ -10,6 +10,7 @@
    > #include <linux/delay.h>
    > #include <linux/device.h>
    > #include <linux/dma-mapping.h>
    > +#include <linux/err.h>
    > #include <linux/errno.h>
    > #include <linux/firewire.h>
    > #include <linux/firewire-cdev.h>
    > @@ -953,11 +954,25 @@ static enum dma_data_direction iso_dma_direction(struct fw_iso_context *context)
    > return DMA_FROM_DEVICE;
    > }
    >
    > +static struct fw_iso_context *fw_iso_mc_context_create(struct fw_card *card,
    > + fw_iso_mc_callback_t callback,
    > + void *callback_data)
    > +{
    > + struct fw_iso_context *ctx;
    > +
    > + ctx = fw_iso_context_create(card, FW_ISO_CONTEXT_RECEIVE_MULTICHANNEL,
    > + 0, 0, 0, NULL, callback_data);
    > + if (!IS_ERR(ctx))
    > + ctx->callback.mc = callback;
    > +
    > + return ctx;
    > +}
    > +
    > static int ioctl_create_iso_context(struct client *client, union ioctl_arg *arg)
    > {
    > struct fw_cdev_create_iso_context *a = &arg->create_iso_context;
    > struct fw_iso_context *context;
    > - fw_iso_callback_t cb;
    > + union fw_iso_callback cb;
    > int ret;
    >
    > BUILD_BUG_ON(FW_CDEV_ISO_CONTEXT_TRANSMIT != FW_ISO_CONTEXT_TRANSMIT ||
    > @@ -970,7 +985,7 @@ static int ioctl_create_iso_context(struct client *client, union ioctl_arg *arg)
    > if (a->speed > SCODE_3200 || a->channel > 63)
    > return -EINVAL;
    >
    > - cb = iso_callback;
    > + cb.sc = iso_callback;
    > break;
    >
    > case FW_ISO_CONTEXT_RECEIVE:
    > @@ -978,19 +993,24 @@ static int ioctl_create_iso_context(struct client *client, union ioctl_arg *arg)
    > a->channel > 63)
    > return -EINVAL;
    >
    > - cb = iso_callback;
    > + cb.sc = iso_callback;
    > break;
    >
    > case FW_ISO_CONTEXT_RECEIVE_MULTICHANNEL:
    > - cb = (fw_iso_callback_t)iso_mc_callback;
    > + cb.mc = iso_mc_callback;
    > break;
    >
    > default:
    > return -EINVAL;
    > }
    >
    > - context = fw_iso_context_create(client->device->card, a->type,
    > - a->channel, a->speed, a->header_size, cb, client);
    > + if (a->type == FW_ISO_CONTEXT_RECEIVE_MULTICHANNEL)
    > + context = fw_iso_mc_context_create(client->device->card, cb.mc,
    > + client);
    > + else
    > + context = fw_iso_context_create(client->device->card, a->type,
    > + a->channel, a->speed,
    > + a->header_size, cb.sc, client);
    > if (IS_ERR(context))
    > return PTR_ERR(context);
    > if (client->version < FW_CDEV_VERSION_AUTO_FLUSH_ISO_OVERFLOW)
    > diff --git a/include/linux/firewire.h b/include/linux/firewire.h
    > index aec8f30ab200..07967a450eaa 100644
    > --- a/include/linux/firewire.h
    > +++ b/include/linux/firewire.h
    > @@ -436,6 +436,12 @@ typedef void (*fw_iso_callback_t)(struct fw_iso_context *context,
    > void *header, void *data);
    > typedef void (*fw_iso_mc_callback_t)(struct fw_iso_context *context,
    > dma_addr_t completed, void *data);
    > +
    > +union fw_iso_callback {
    > + fw_iso_callback_t sc;
    > + fw_iso_mc_callback_t mc;
    > +};
    > +
    > struct fw_iso_context {
    > struct fw_card *card;
    > int type;
    > @@ -443,10 +449,7 @@ struct fw_iso_context {
    > int speed;
    > bool drop_overflow_headers;
    > size_t header_size;
    > - union {
    > - fw_iso_callback_t sc;
    > - fw_iso_mc_callback_t mc;
    > - } callback;
    > + union fw_iso_callback callback;
    > void *callback_data;
    > };
    >
    > --
    > 2.20.1
    >

    Regards,
    Oscar Carter

    \
     
     \ /
      Last update: 2020-07-04 18:16    [W:3.360 / U:1.124 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site