Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 25 May 2020 10:44:58 +0900
From:   Takashi Sakamoto <o-takashi@sakamocchi.jp>
To:     Stefan Richter <stefanr@s5r6.in-berlin.de>
Cc:     Greg KH <greg@kroah.com>, oscar.carter@gmx.com,
        keescook@chromium.org, kernel-hardening@lists.openwall.com,
        linux1394-devel@lists.sourceforge.net,
        linux-kernel@vger.kernel.org, clemens@ladisch.de
Subject: Re: [PATCH v2] firewire-core: remove cast of function callback
Message-ID: <20200525014458.GA252667@workstation>
Mail-Followup-To: Stefan Richter <stefanr@s5r6.in-berlin.de>,
        Greg KH <greg@kroah.com>, oscar.carter@gmx.com,
        keescook@chromium.org, kernel-hardening@lists.openwall.com,
        linux1394-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org,
        clemens@ladisch.de
References: <20200524132048.243223-1-o-takashi@sakamocchi.jp>
 <20200524152301.GB21163@kroah.com>
 <20200525015532.0055f9df@kant>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200525015532.0055f9df@kant>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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 = &arg->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