Re: [PATCH v2] firewire-core: remove cast of function callback
Hi,
On Mon, May 25, 2020 at 01:55:32AM +0200, Stefan Richter wrote:
> > Why is this in a .h file? What's wrong with just putting it in the .c
> > file as a static function? There's no need to make this an inline,
> > right?
>
> There is no need for this in a header.
> Furthermore, I prefer the original switch statement over the nested if/else.
>
> Here is another proposal; I will resend it later as a proper patch.
>
> diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
> index 719791819c24..bece1b69b43f 100644
> --- a/drivers/firewire/core-cdev.c
> +++ b/drivers/firewire/core-cdev.c
> @@ -957,7 +957,6 @@ static int ioctl_create_iso_context(struct client
> *client, union ioctl_arg *arg)
> {
> struct fw_cdev_create_iso_context *a = >create_iso_context;
> struct fw_iso_context *context;
> - fw_iso_callback_t cb;
> int ret;
>
> BUILD_BUG_ON(FW_CDEV_ISO_CONTEXT_TRANSMIT != FW_ISO_CONTEXT_TRANSMIT ||
> @@ -969,20 +968,15 @@ static int ioctl_create_iso_context(struct client
> *client, union ioctl_arg *arg)
> case FW_ISO_CONTEXT_TRANSMIT:
> if (a->speed > SCODE_3200 || a->channel > 63)
> return -EINVAL;
> -
> - cb = iso_callback;
> break;
>
> case FW_ISO_CONTEXT_RECEIVE:
> if (a->header_size < 4 || (a->header_size & 3) ||
> a->channel > 63)
> return -EINVAL;
> -
> - cb = iso_callback;
> break;
>
> case FW_ISO_CONTEXT_RECEIVE_MULTICHANNEL:
> - cb = (fw_iso_callback_t)iso_mc_callback;
> break;
>
> default:
> @@ -990,9 +984,15 @@ static int ioctl_create_iso_context(struct client
> *client, union ioctl_arg *arg)
> }
>
> context = fw_iso_context_create(client->device->card, a->type,
> - a->channel, a->speed, a->header_size, cb, client);
> + a->channel, a->speed, a->header_size, NULL, client);
> if (IS_ERR(context))
> return PTR_ERR(context);
> +
> + if (a->type == FW_ISO_CONTEXT_RECEIVE_MULTICHANNEL)
> + context->callback.mc = iso_mc_callback;
> + else
> + context->callback.sc = iso_callback;
> +
> if (client->version < FW_CDEV_VERSION_AUTO_FLUSH_ISO_OVERFLOW)
> context->drop_overflow_headers = true;
At first place, I wrote the similar patch but judged it's a bit ad-hoc
way that callback functions are assigned after the call of
fw_iso_context_create() in raw code. For explicitness in a point of things
being declarative, I put the inline function into header, and avoid
someone's misfortune for future even if IEEE 1394 is enough legacy.
Anyway, I don't mind Stefan's proposal since it works well. It depends
on developers' fashion to choose policy to write codes.
Thanks
Takashi Sakamoto
Re: [PATCH v2] firewire-core: remove cast of function callback
On May 24 Greg KH wrote:
> On Sun, May 24, 2020 at 10:20:48PM +0900, Takashi Sakamoto 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 inline 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, 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.
> >
> > Changes in v2:
> > - unexport helper function
> > - use inline for helper function
> > - arrange arguments for helper function
> > - tested by libhinoko
> >
> > Reported-by: Oscar Carter
> > Reference:
> > https://lore.kernel.org/lkml/20200519173425.4724-1-oscar.car...@gmx.com/
> > Signed-off-by: Takashi Sakamoto
> > ---
> > drivers/firewire/core-cdev.c | 40 +++-
> > include/linux/firewire.h | 16 +++
> > 2 files changed, 33 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
> > index 6e291d8f3a27..7cbf6df34b43 100644
> > --- a/drivers/firewire/core-cdev.c
> > +++ b/drivers/firewire/core-cdev.c
> > @@ -957,7 +957,6 @@ static int ioctl_create_iso_context(struct client
> > *client, union ioctl_arg *arg)
> > {
> > struct fw_cdev_create_iso_context *a = >create_iso_context;
> > struct fw_iso_context *context;
> > - fw_iso_callback_t cb;
> > int ret;
> >
> > BUILD_BUG_ON(FW_CDEV_ISO_CONTEXT_TRANSMIT != FW_ISO_CONTEXT_TRANSMIT ||
> > @@ -965,32 +964,27 @@ static int ioctl_create_iso_context(struct client
> > *client, union ioctl_arg *arg)
> > FW_CDEV_ISO_CONTEXT_RECEIVE_MULTICHANNEL !=
> > FW_ISO_CONTEXT_RECEIVE_MULTICHANNEL);
> >
> > - switch (a->type) {
> > - case FW_ISO_CONTEXT_TRANSMIT:
> > - if (a->speed > SCODE_3200 || a->channel > 63)
> > - return -EINVAL;
> > -
> > - cb = iso_callback;
> > - break;
> > -
> > - case FW_ISO_CONTEXT_RECEIVE:
> > - if (a->header_size < 4 || (a->header_size & 3) ||
> > - a->channel > 63)
> > - return -EINVAL;
> > -
> > - cb = iso_callback;
> > - break;
> > -
> > - case FW_ISO_CONTEXT_RECEIVE_MULTICHANNEL:
> > - cb = (fw_iso_callback_t)iso_mc_callback;
> > - break;
> > + if (a->type == FW_ISO_CONTEXT_TRANSMIT ||
> > + a->type == FW_ISO_CONTEXT_RECEIVE) {
> > + if (a->type == FW_ISO_CONTEXT_TRANSMIT) {
> > + if (a->speed > SCODE_3200 || a->channel > 63)
> > + return -EINVAL;
> > + } else {
> > + if (a->header_size < 4 || (a->header_size & 3) ||
> > + a->channel > 63)
> > + return -EINVAL;
> > + }
> >
> > - default:
> > + context = fw_iso_context_create(client->device->card, a->type,
> > + a->channel, a->speed, a->header_size,
> > + iso_callback, client);
> > + } else if (a->type == FW_ISO_CONTEXT_RECEIVE_MULTICHANNEL) {
> > + context = fw_iso_mc_context_create(client->device->card,
> > + iso_mc_callback, client);
> > + } else {
> > return -EINVAL;
> > }
> >
> > - context = fw_iso_context_create(client->device->card, a->type,
> > - a->channel, a->speed, a->header_size, cb, 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..bff08118baaf 100644
> > --- a/include/linux/firewire.h
> > +++ b/include/linux/firewire.h
> > @@ -453,6 +453,22 @@ struct fw_iso_context {
> > struct fw_iso_context *fw_iso_context_create(struct fw_card *card,
> > int type, int channel, int speed, size_t header_size,
> > fw_iso_callback_t callback, void *callback_data);
> > +
> > +static inline struct fw_iso_context
Re: [PATCH v2] firewire-core: remove cast of function callback
On Sun, May 24, 2020 at 10:20:48PM +0900, Takashi Sakamoto 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 inline 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, 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.
>
> Changes in v2:
> - unexport helper function
> - use inline for helper function
> - arrange arguments for helper function
> - tested by libhinoko
>
> Reported-by: Oscar Carter
> Reference:
> https://lore.kernel.org/lkml/20200519173425.4724-1-oscar.car...@gmx.com/
> Signed-off-by: Takashi Sakamoto
> ---
> drivers/firewire/core-cdev.c | 40 +++-
> include/linux/firewire.h | 16 +++
> 2 files changed, 33 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
> index 6e291d8f3a27..7cbf6df34b43 100644
> --- a/drivers/firewire/core-cdev.c
> +++ b/drivers/firewire/core-cdev.c
> @@ -957,7 +957,6 @@ static int ioctl_create_iso_context(struct client
> *client, union ioctl_arg *arg)
> {
> struct fw_cdev_create_iso_context *a = >create_iso_context;
> struct fw_iso_context *context;
> - fw_iso_callback_t cb;
> int ret;
>
> BUILD_BUG_ON(FW_CDEV_ISO_CONTEXT_TRANSMIT != FW_ISO_CONTEXT_TRANSMIT ||
> @@ -965,32 +964,27 @@ static int ioctl_create_iso_context(struct client
> *client, union ioctl_arg *arg)
>FW_CDEV_ISO_CONTEXT_RECEIVE_MULTICHANNEL !=
> FW_ISO_CONTEXT_RECEIVE_MULTICHANNEL);
>
> - switch (a->type) {
> - case FW_ISO_CONTEXT_TRANSMIT:
> - if (a->speed > SCODE_3200 || a->channel > 63)
> - return -EINVAL;
> -
> - cb = iso_callback;
> - break;
> -
> - case FW_ISO_CONTEXT_RECEIVE:
> - if (a->header_size < 4 || (a->header_size & 3) ||
> - a->channel > 63)
> - return -EINVAL;
> -
> - cb = iso_callback;
> - break;
> -
> - case FW_ISO_CONTEXT_RECEIVE_MULTICHANNEL:
> - cb = (fw_iso_callback_t)iso_mc_callback;
> - break;
> + if (a->type == FW_ISO_CONTEXT_TRANSMIT ||
> + a->type == FW_ISO_CONTEXT_RECEIVE) {
> + if (a->type == FW_ISO_CONTEXT_TRANSMIT) {
> + if (a->speed > SCODE_3200 || a->channel > 63)
> + return -EINVAL;
> + } else {
> + if (a->header_size < 4 || (a->header_size & 3) ||
> + a->channel > 63)
> + return -EINVAL;
> + }
>
> - default:
> + context = fw_iso_context_create(client->device->card, a->type,
> + a->channel, a->speed, a->header_size,
> + iso_callback, client);
> + } else if (a->type == FW_ISO_CONTEXT_RECEIVE_MULTICHANNEL) {
> + context = fw_iso_mc_context_create(client->device->card,
> +iso_mc_callback, client);
> + } else {
> return -EINVAL;
> }
>
> - context = fw_iso_context_create(client->device->card, a->type,
> - a->channel, a->speed, a->header_size, cb, 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..bff08118baaf 100644
> --- a/include/linux/firewire.h
> +++ b/include/linux/firewire.h
> @@ -453,6 +453,22 @@ struct fw_iso_context {
> struct fw_iso_context *fw_iso_context_create(struct fw_card *card,
> int type, int channel, int speed, size_t header_size,
> fw_iso_callback_t callback, void *callback_data);
> +
> +static inline struct fw_iso_context *fw_iso_mc_context_create(
> + struct fw_card *card,
> +
