| Message ID | 20200530090839.7895-1-oscar.carter@gmx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] firewire: Remove function callback casts | expand |
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
Hi, I'm sorry to be late but I was stuck at my work for ALSA control service programs for audio and music units on IEEE 1394 bus[1]. 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. In my opinion, it's no need to add my and Stefan's sign-off tag to patch in which you firstly wrote even if it includes ideas from the others ;) > 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(-) Anyway this patch looks good to me. I test this patch with libhinoko and find no regression. Reviewed-by: Takashi Sakamoto <o-takashi@sakamocchi.jp> Testeb-by: Takashi Sakamoto<o-takashi@sakamocchi.jp> [1] [RFT] ALSA control service programs for Digidesign Digi 002/003 family and Tascam FireWire series https://mailman.alsa-project.org/pipermail/alsa-devel/2020-July/170331.html Thanks Takashi Sakamoto
Hi Takashi, On Wed, Jul 08, 2020 at 10:06:28PM +0900, Takashi Sakamoto wrote: > Hi, > > I'm sorry to be late but I was stuck at my work for ALSA control > service programs for audio and music units on IEEE 1394 bus[1]. > > On Sat, May 30, 2020 at 11:08:39AM +0200, Oscar Carter wrote: > > [...] > > 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. > > In my opinion, it's no need to add my and Stefan's sign-off tag to patch > in which you firstly wrote even if it includes ideas from the others ;) I would like to leave it as is because most of the work is based on your code (Takashi and Stefan). > > [...] > > Anyway this patch looks good to me. I test this patch with libhinoko and > find no regression. > > Reviewed-by: Takashi Sakamoto <o-takashi@sakamocchi.jp> > Testeb-by: Takashi Sakamoto<o-takashi@sakamocchi.jp> > > > [1] [RFT] ALSA control service programs for Digidesign Digi 002/003 family > and Tascam FireWire series > https://mailman.alsa-project.org/pipermail/alsa-devel/2020-July/170331.html > > Thanks > > Takashi Sakamoto Regards, Oscar Carter
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; };