public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCHv2 1/2] usb: gadget: f_midi: use flexible array member for gmidi_in_port elements
@ 2016-01-09  3:47 Michal Nazarewicz
  2016-01-09  3:47 ` [PATCH 2/2] usb: gadget: f_midi: missing unlock on error path Michal Nazarewicz
  2016-01-13 18:26 ` [PATCHv2 1/2] usb: gadget: f_midi: use flexible array member for gmidi_in_port elements Felipe Ferreri Tonello
  0 siblings, 2 replies; 7+ messages in thread
From: Michal Nazarewicz @ 2016-01-09  3:47 UTC (permalink / raw)
  To: Felipe Balbi, Greg Kroah-Hartman, Dan Carpenter
  Cc: Felipe F. Tonello, Robert Baldyga, Andrzej Pietrasiewicz,
	Pawel Szewczyk, linux-kernel, Michal Nazarewicz

Reduce number of allocations, simplify memory management and reduce
memory usage by stacking the gmidi_in_port elements at the end of the
f_midi structure using a flexible array.

Also, observe that gmidi_in_port::midi pointer is *never* used for any
purpose so it can be safely removed.

Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
 drivers/usb/gadget/function/f_midi.c | 43 ++++++++++++------------------------
 1 file changed, 14 insertions(+), 29 deletions(-)

On Tue, Jan 05 2016, Dan Carpenter wrote:
> On Tue, Jan 05, 2016 at 02:55:31PM +0100, Michal Nazarewicz wrote:
>> @@ -568,12 +568,12 @@ static int f_midi_in_open(struct snd_rawmidi_substream *substream)
>>  {
>>  	struct f_midi *midi = substream->rmidi->private_data;
>>  
>> -	if (!midi->in_port[substream->number])
>> +	if (substream->number > midi->in_ports)
>
> This is off by one.  It should be >= midi->in_ports.

Fixed.

Since there were no objections to this small patch set (i.e. there was
mostly silence about it), I’ll go ahead and interpret it as ‘go ahead
and properly submit it’. :)

diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
index 898a570..fd67af2 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -55,7 +55,6 @@ static const char f_midi_longname[] = "MIDI Gadget";
  * USB <- IN endpoint  <- rawmidi
  */
 struct gmidi_in_port {
-	struct f_midi *midi;
 	int active;
 	uint8_t cable;
 	uint8_t state;
@@ -78,7 +77,6 @@ struct f_midi {
 
 	struct snd_rawmidi_substream *in_substream[MAX_PORTS];
 	struct snd_rawmidi_substream *out_substream[MAX_PORTS];
-	struct gmidi_in_port	*in_port[MAX_PORTS];
 
 	unsigned long		out_triggered;
 	struct tasklet_struct	tasklet;
@@ -87,6 +85,8 @@ struct f_midi {
 	int index;
 	char *id;
 	unsigned int buflen, qlen;
+
+	struct gmidi_in_port	in_ports_array[/* in_ports */];
 };
 
 static inline struct f_midi *func_to_midi(struct usb_function *f)
@@ -529,11 +529,11 @@ static void f_midi_transmit(struct f_midi *midi, struct usb_request *req)
 	req->length = 0;
 	req->complete = f_midi_complete;
 
-	for (i = 0; i < MAX_PORTS; i++) {
-		struct gmidi_in_port *port = midi->in_port[i];
+	for (i = 0; i < midi->in_ports; i++) {
+		struct gmidi_in_port *port = midi->in_ports_array + i;
 		struct snd_rawmidi_substream *substream = midi->in_substream[i];
 
-		if (!port || !port->active || !substream)
+		if (!port->active || !substream)
 			continue;
 
 		while (req->length + 3 < midi->buflen) {
@@ -568,12 +568,12 @@ static int f_midi_in_open(struct snd_rawmidi_substream *substream)
 {
 	struct f_midi *midi = substream->rmidi->private_data;
 
-	if (!midi->in_port[substream->number])
+	if (substream->number >= midi->in_ports)
 		return -EINVAL;
 
 	VDBG(midi, "%s()\n", __func__);
 	midi->in_substream[substream->number] = substream;
-	midi->in_port[substream->number]->state = STATE_UNKNOWN;
+	midi->in_ports_array[substream->number].state = STATE_UNKNOWN;
 	return 0;
 }
 
@@ -589,11 +589,11 @@ static void f_midi_in_trigger(struct snd_rawmidi_substream *substream, int up)
 {
 	struct f_midi *midi = substream->rmidi->private_data;
 
-	if (!midi->in_port[substream->number])
+	if (substream->number >= midi->in_ports)
 		return;
 
 	VDBG(midi, "%s() %d\n", __func__, up);
-	midi->in_port[substream->number]->active = up;
+	midi->in_ports_array[substream->number].active = up;
 	if (up)
 		tasklet_hi_schedule(&midi->tasklet);
 }
@@ -1067,14 +1067,11 @@ static void f_midi_free(struct usb_function *f)
 {
 	struct f_midi *midi;
 	struct f_midi_opts *opts;
-	int i;
 
 	midi = func_to_midi(f);
 	opts = container_of(f->fi, struct f_midi_opts, func_inst);
 	kfree(midi->id);
 	mutex_lock(&opts->lock);
-	for (i = opts->in_ports - 1; i >= 0; --i)
-		kfree(midi->in_port[i]);
 	kfree(midi);
 	--opts->refcnt;
 	mutex_unlock(&opts->lock);
@@ -1115,26 +1112,16 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
 	}
 
 	/* allocate and initialize one new instance */
-	midi = kzalloc(sizeof(*midi), GFP_KERNEL);
+	midi = kzalloc(
+		sizeof(*midi) + opts->in_ports * sizeof(*midi->in_ports_array),
+		GFP_KERNEL);
 	if (!midi) {
 		mutex_unlock(&opts->lock);
 		return ERR_PTR(-ENOMEM);
 	}
 
-	for (i = 0; i < opts->in_ports; i++) {
-		struct gmidi_in_port *port = kzalloc(sizeof(*port), GFP_KERNEL);
-
-		if (!port) {
-			status = -ENOMEM;
-			mutex_unlock(&opts->lock);
-			goto setup_fail;
-		}
-
-		port->midi = midi;
-		port->active = 0;
-		port->cable = i;
-		midi->in_port[i] = port;
-	}
+	for (i = 0; i < opts->in_ports; i++)
+		midi->in_ports_array[i].cable = i;
 
 	/* set up ALSA midi devices */
 	midi->id = kstrdup(opts->id, GFP_KERNEL);
@@ -1161,8 +1148,6 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
 	return &midi->func;
 
 setup_fail:
-	for (--i; i >= 0; i--)
-		kfree(midi->in_port[i]);
 	kfree(midi);
 	return ERR_PTR(status);
 }
-- 
2.6.0.rc2.230.g3dd15c0

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 2/2] usb: gadget: f_midi: missing unlock on error path
  2016-01-09  3:47 [PATCHv2 1/2] usb: gadget: f_midi: use flexible array member for gmidi_in_port elements Michal Nazarewicz
@ 2016-01-09  3:47 ` Michal Nazarewicz
  2016-01-13 18:29   ` Felipe Ferreri Tonello
  2016-01-13 18:26 ` [PATCHv2 1/2] usb: gadget: f_midi: use flexible array member for gmidi_in_port elements Felipe Ferreri Tonello
  1 sibling, 1 reply; 7+ messages in thread
From: Michal Nazarewicz @ 2016-01-09  3:47 UTC (permalink / raw)
  To: Felipe Balbi, Greg Kroah-Hartman, Dan Carpenter
  Cc: Felipe F. Tonello, Robert Baldyga, Andrzej Pietrasiewicz,
	Pawel Szewczyk, linux-kernel, Michal Nazarewicz

From: Dan Carpenter <dan.carpenter@oracle.com>

We added a new error path to this function and we forgot to drop the
lock.

Fixes: e1e3d7ec5da3 ('usb: gadget: f_midi: pre-allocate IN requests')
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
[mina86@mina86.com: rebased on top of refactoring commit]
Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
 drivers/usb/gadget/function/f_midi.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
index fd67af2..f287c68 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -1098,7 +1098,7 @@ static void f_midi_unbind(struct usb_configuration *c, struct usb_function *f)
 
 static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
 {
-	struct f_midi *midi;
+	struct f_midi *midi = NULL;
 	struct f_midi_opts *opts;
 	int status, i;
 
@@ -1107,8 +1107,8 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
 	mutex_lock(&opts->lock);
 	/* sanity check */
 	if (opts->in_ports > MAX_PORTS || opts->out_ports > MAX_PORTS) {
-		mutex_unlock(&opts->lock);
-		return ERR_PTR(-EINVAL);
+		status = -EINVAL;
+		goto setup_fail;
 	}
 
 	/* allocate and initialize one new instance */
@@ -1116,8 +1116,8 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
 		sizeof(*midi) + opts->in_ports * sizeof(*midi->in_ports_array),
 		GFP_KERNEL);
 	if (!midi) {
-		mutex_unlock(&opts->lock);
-		return ERR_PTR(-ENOMEM);
+		status = -ENOMEM;
+		goto setup_fail;
 	}
 
 	for (i = 0; i < opts->in_ports; i++)
@@ -1127,7 +1127,6 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
 	midi->id = kstrdup(opts->id, GFP_KERNEL);
 	if (opts->id && !midi->id) {
 		status = -ENOMEM;
-		mutex_unlock(&opts->lock);
 		goto setup_fail;
 	}
 	midi->in_ports = opts->in_ports;
@@ -1148,6 +1147,7 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
 	return &midi->func;
 
 setup_fail:
+	mutex_unlock(&opts->lock);
 	kfree(midi);
 	return ERR_PTR(status);
 }
-- 
2.6.0.rc2.230.g3dd15c0

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCHv2 1/2] usb: gadget: f_midi: use flexible array member for gmidi_in_port elements
  2016-01-09  3:47 [PATCHv2 1/2] usb: gadget: f_midi: use flexible array member for gmidi_in_port elements Michal Nazarewicz
  2016-01-09  3:47 ` [PATCH 2/2] usb: gadget: f_midi: missing unlock on error path Michal Nazarewicz
@ 2016-01-13 18:26 ` Felipe Ferreri Tonello
  2016-01-13 20:41   ` Dan Carpenter
  2016-01-18 16:02   ` Michal Nazarewicz
  1 sibling, 2 replies; 7+ messages in thread
From: Felipe Ferreri Tonello @ 2016-01-13 18:26 UTC (permalink / raw)
  To: Michal Nazarewicz, Felipe Balbi, Greg Kroah-Hartman,
	Dan Carpenter
  Cc: Robert Baldyga, Andrzej Pietrasiewicz, Pawel Szewczyk,
	linux-kernel

[-- Attachment #1: Type: text/plain, Size: 5489 bytes --]

Hi Michael,

On 09/01/16 03:47, Michal Nazarewicz wrote:
> Reduce number of allocations, simplify memory management and reduce
> memory usage by stacking the gmidi_in_port elements at the end of the
> f_midi structure using a flexible array.
> 
> Also, observe that gmidi_in_port::midi pointer is *never* used for any
> purpose so it can be safely removed.
> 
> Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
> ---
>  drivers/usb/gadget/function/f_midi.c | 43 ++++++++++++------------------------
>  1 file changed, 14 insertions(+), 29 deletions(-)
> 
> On Tue, Jan 05 2016, Dan Carpenter wrote:
>> On Tue, Jan 05, 2016 at 02:55:31PM +0100, Michal Nazarewicz wrote:
>>> @@ -568,12 +568,12 @@ static int f_midi_in_open(struct snd_rawmidi_substream *substream)
>>>  {
>>>  	struct f_midi *midi = substream->rmidi->private_data;
>>>  
>>> -	if (!midi->in_port[substream->number])
>>> +	if (substream->number > midi->in_ports)
>>
>> This is off by one.  It should be >= midi->in_ports.
> 
> Fixed.
> 
> Since there were no objections to this small patch set (i.e. there was
> mostly silence about it), I’ll go ahead and interpret it as ‘go ahead
> and properly submit it’. :)
> 
> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
> index 898a570..fd67af2 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -55,7 +55,6 @@ static const char f_midi_longname[] = "MIDI Gadget";
>   * USB <- IN endpoint  <- rawmidi
>   */
>  struct gmidi_in_port {
> -	struct f_midi *midi;

This change is unrelated. I sent a patch removing this pointer as well.

>  	int active;
>  	uint8_t cable;
>  	uint8_t state;
> @@ -78,7 +77,6 @@ struct f_midi {
>  
>  	struct snd_rawmidi_substream *in_substream[MAX_PORTS];
>  	struct snd_rawmidi_substream *out_substream[MAX_PORTS];
> -	struct gmidi_in_port	*in_port[MAX_PORTS];
>  
>  	unsigned long		out_triggered;
>  	struct tasklet_struct	tasklet;
> @@ -87,6 +85,8 @@ struct f_midi {
>  	int index;
>  	char *id;
>  	unsigned int buflen, qlen;
> +
> +	struct gmidi_in_port	in_ports_array[/* in_ports */];
>  };
>  
>  static inline struct f_midi *func_to_midi(struct usb_function *f)
> @@ -529,11 +529,11 @@ static void f_midi_transmit(struct f_midi *midi, struct usb_request *req)
>  	req->length = 0;
>  	req->complete = f_midi_complete;
>  
> -	for (i = 0; i < MAX_PORTS; i++) {
> -		struct gmidi_in_port *port = midi->in_port[i];
> +	for (i = 0; i < midi->in_ports; i++) {
> +		struct gmidi_in_port *port = midi->in_ports_array + i;
>  		struct snd_rawmidi_substream *substream = midi->in_substream[i];
>  
> -		if (!port || !port->active || !substream)
> +		if (!port->active || !substream)
>  			continue;
>  
>  		while (req->length + 3 < midi->buflen) {
> @@ -568,12 +568,12 @@ static int f_midi_in_open(struct snd_rawmidi_substream *substream)
>  {
>  	struct f_midi *midi = substream->rmidi->private_data;
>  
> -	if (!midi->in_port[substream->number])
> +	if (substream->number >= midi->in_ports)
>  		return -EINVAL;
>  
>  	VDBG(midi, "%s()\n", __func__);
>  	midi->in_substream[substream->number] = substream;
> -	midi->in_port[substream->number]->state = STATE_UNKNOWN;
> +	midi->in_ports_array[substream->number].state = STATE_UNKNOWN;
>  	return 0;
>  }
>  
> @@ -589,11 +589,11 @@ static void f_midi_in_trigger(struct snd_rawmidi_substream *substream, int up)
>  {
>  	struct f_midi *midi = substream->rmidi->private_data;
>  
> -	if (!midi->in_port[substream->number])
> +	if (substream->number >= midi->in_ports)
>  		return;
>  
>  	VDBG(midi, "%s() %d\n", __func__, up);
> -	midi->in_port[substream->number]->active = up;
> +	midi->in_ports_array[substream->number].active = up;
>  	if (up)
>  		tasklet_hi_schedule(&midi->tasklet);
>  }
> @@ -1067,14 +1067,11 @@ static void f_midi_free(struct usb_function *f)
>  {
>  	struct f_midi *midi;
>  	struct f_midi_opts *opts;
> -	int i;
>  
>  	midi = func_to_midi(f);
>  	opts = container_of(f->fi, struct f_midi_opts, func_inst);
>  	kfree(midi->id);
>  	mutex_lock(&opts->lock);
> -	for (i = opts->in_ports - 1; i >= 0; --i)
> -		kfree(midi->in_port[i]);
>  	kfree(midi);
>  	--opts->refcnt;
>  	mutex_unlock(&opts->lock);
> @@ -1115,26 +1112,16 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
>  	}
>  
>  	/* allocate and initialize one new instance */
> -	midi = kzalloc(sizeof(*midi), GFP_KERNEL);
> +	midi = kzalloc(
> +		sizeof(*midi) + opts->in_ports * sizeof(*midi->in_ports_array),
> +		GFP_KERNEL);

Is there a garantee that the compiler will always use in_ports_array at
the end of the allocated block?

>  	if (!midi) {
>  		mutex_unlock(&opts->lock);
>  		return ERR_PTR(-ENOMEM);
>  	}
>  
> -	for (i = 0; i < opts->in_ports; i++) {
> -		struct gmidi_in_port *port = kzalloc(sizeof(*port), GFP_KERNEL);
> -
> -		if (!port) {
> -			status = -ENOMEM;
> -			mutex_unlock(&opts->lock);
> -			goto setup_fail;
> -		}
> -
> -		port->midi = midi;
> -		port->active = 0;
> -		port->cable = i;
> -		midi->in_port[i] = port;
> -	}
> +	for (i = 0; i < opts->in_ports; i++)
> +		midi->in_ports_array[i].cable = i;
>  
>  	/* set up ALSA midi devices */
>  	midi->id = kstrdup(opts->id, GFP_KERNEL);
> @@ -1161,8 +1148,6 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
>  	return &midi->func;
>  
>  setup_fail:
> -	for (--i; i >= 0; i--)
> -		kfree(midi->in_port[i]);
>  	kfree(midi);
>  	return ERR_PTR(status);
>  }
> 

Felipe

[-- Attachment #2: 0x92698E6A.asc --]
[-- Type: application/pgp-keys, Size: 7310 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] usb: gadget: f_midi: missing unlock on error path
  2016-01-09  3:47 ` [PATCH 2/2] usb: gadget: f_midi: missing unlock on error path Michal Nazarewicz
@ 2016-01-13 18:29   ` Felipe Ferreri Tonello
  0 siblings, 0 replies; 7+ messages in thread
From: Felipe Ferreri Tonello @ 2016-01-13 18:29 UTC (permalink / raw)
  To: Michal Nazarewicz, Felipe Balbi, Greg Kroah-Hartman,
	Dan Carpenter
  Cc: Robert Baldyga, Andrzej Pietrasiewicz, Pawel Szewczyk,
	linux-kernel

[-- Attachment #1: Type: text/plain, Size: 2392 bytes --]

Hi Dan,

On 09/01/16 03:47, Michal Nazarewicz wrote:
> From: Dan Carpenter <dan.carpenter@oracle.com>
> 
> We added a new error path to this function and we forgot to drop the
> lock.
> 
> Fixes: e1e3d7ec5da3 ('usb: gadget: f_midi: pre-allocate IN requests')
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> [mina86@mina86.com: rebased on top of refactoring commit]
> Signed-off-by: Michal Nazarewicz <mina86@mina86.com>

Reviewed-by: Felipe F. Tonello <eu@felipetonello.com>

> ---
>  drivers/usb/gadget/function/f_midi.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
> index fd67af2..f287c68 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -1098,7 +1098,7 @@ static void f_midi_unbind(struct usb_configuration *c, struct usb_function *f)
>  
>  static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
>  {
> -	struct f_midi *midi;
> +	struct f_midi *midi = NULL;
>  	struct f_midi_opts *opts;
>  	int status, i;
>  
> @@ -1107,8 +1107,8 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
>  	mutex_lock(&opts->lock);
>  	/* sanity check */
>  	if (opts->in_ports > MAX_PORTS || opts->out_ports > MAX_PORTS) {
> -		mutex_unlock(&opts->lock);
> -		return ERR_PTR(-EINVAL);
> +		status = -EINVAL;
> +		goto setup_fail;
>  	}
>  
>  	/* allocate and initialize one new instance */
> @@ -1116,8 +1116,8 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
>  		sizeof(*midi) + opts->in_ports * sizeof(*midi->in_ports_array),
>  		GFP_KERNEL);
>  	if (!midi) {
> -		mutex_unlock(&opts->lock);
> -		return ERR_PTR(-ENOMEM);
> +		status = -ENOMEM;
> +		goto setup_fail;
>  	}
>  
>  	for (i = 0; i < opts->in_ports; i++)
> @@ -1127,7 +1127,6 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
>  	midi->id = kstrdup(opts->id, GFP_KERNEL);
>  	if (opts->id && !midi->id) {
>  		status = -ENOMEM;
> -		mutex_unlock(&opts->lock);
>  		goto setup_fail;
>  	}
>  	midi->in_ports = opts->in_ports;
> @@ -1148,6 +1147,7 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
>  	return &midi->func;
>  
>  setup_fail:
> +	mutex_unlock(&opts->lock);
>  	kfree(midi);
>  	return ERR_PTR(status);
>  }
> 

[-- Attachment #2: 0x92698E6A.asc --]
[-- Type: application/pgp-keys, Size: 7195 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCHv2 1/2] usb: gadget: f_midi: use flexible array member for gmidi_in_port elements
  2016-01-13 18:26 ` [PATCHv2 1/2] usb: gadget: f_midi: use flexible array member for gmidi_in_port elements Felipe Ferreri Tonello
@ 2016-01-13 20:41   ` Dan Carpenter
  2016-01-18 16:02   ` Michal Nazarewicz
  1 sibling, 0 replies; 7+ messages in thread
From: Dan Carpenter @ 2016-01-13 20:41 UTC (permalink / raw)
  To: Felipe Ferreri Tonello
  Cc: Michal Nazarewicz, Felipe Balbi, Greg Kroah-Hartman,
	Robert Baldyga, Andrzej Pietrasiewicz, Pawel Szewczyk,
	linux-kernel

On Wed, Jan 13, 2016 at 06:26:22PM +0000, Felipe Ferreri Tonello wrote:
> > @@ -1115,26 +1112,16 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
> >  	}
> >  
> >  	/* allocate and initialize one new instance */
> > -	midi = kzalloc(sizeof(*midi), GFP_KERNEL);
> > +	midi = kzalloc(
> > +		sizeof(*midi) + opts->in_ports * sizeof(*midi->in_ports_array),
> > +		GFP_KERNEL);
> 
> Is there a garantee that the compiler will always use in_ports_array at
> the end of the allocated block?
> 

Yes.  It's fine.  Zero size arrays at the end of a struct are normal.

regards,
dan carpenter

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCHv2 1/2] usb: gadget: f_midi: use flexible array member for gmidi_in_port elements
  2016-01-13 18:26 ` [PATCHv2 1/2] usb: gadget: f_midi: use flexible array member for gmidi_in_port elements Felipe Ferreri Tonello
  2016-01-13 20:41   ` Dan Carpenter
@ 2016-01-18 16:02   ` Michal Nazarewicz
  2016-01-19  9:46     ` Felipe Ferreri Tonello
  1 sibling, 1 reply; 7+ messages in thread
From: Michal Nazarewicz @ 2016-01-18 16:02 UTC (permalink / raw)
  To: Felipe Ferreri Tonello, Felipe Balbi, Greg Kroah-Hartman,
	Dan Carpenter
  Cc: Robert Baldyga, Andrzej Pietrasiewicz, Pawel Szewczyk,
	linux-kernel

> On 09/01/16 03:47, Michal Nazarewicz wrote:
>> @@ -55,7 +55,6 @@ static const char f_midi_longname[] = "MIDI Gadget";
>>   * USB <- IN endpoint  <- rawmidi
>>   */
>>  struct gmidi_in_port {
>> -	struct f_midi *midi;

On Wed, Jan 13 2016, Felipe Ferreri Tonello wrote:
> This change is unrelated. I sent a patch removing this pointer as well.

Has it been merged yet?  Which branch?  Could you point me to the patch?

>>  	int active;
>>  	uint8_t cable;
>>  	uint8_t state;
>> @@ -1115,26 +1112,16 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
>>  	}
>>  
>>  	/* allocate and initialize one new instance */
>> -	midi = kzalloc(sizeof(*midi), GFP_KERNEL);
>> +	midi = kzalloc(
>> +		sizeof(*midi) + opts->in_ports * sizeof(*midi->in_ports_array),
>> +		GFP_KERNEL);
>
> Is there a garantee that the compiler will always use in_ports_array at
> the end of the allocated block?

Yep, see <https://en.wikipedia.org/wiki/Flexible_array_member>.

>
>>  	if (!midi) {
>>  		mutex_unlock(&opts->lock);
>>  		return ERR_PTR(-ENOMEM);
>>  	}
>>  
>> -	for (i = 0; i < opts->in_ports; i++) {
>> -		struct gmidi_in_port *port = kzalloc(sizeof(*port), GFP_KERNEL);
>> -
>> -		if (!port) {
>> -			status = -ENOMEM;
>> -			mutex_unlock(&opts->lock);
>> -			goto setup_fail;
>> -		}
>> -
>> -		port->midi = midi;
>> -		port->active = 0;
>> -		port->cable = i;
>> -		midi->in_port[i] = port;
>> -	}
>> +	for (i = 0; i < opts->in_ports; i++)
>> +		midi->in_ports_array[i].cable = i;
>>  
>>  	/* set up ALSA midi devices */
>>  	midi->id = kstrdup(opts->id, GFP_KERNEL);

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  ミハウ “mina86” ナザレヴイツ  (o o)
ooo +--<mpn@google.com>--<xmpp:mina86@jabber.org>--ooO--(_)--Ooo--

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCHv2 1/2] usb: gadget: f_midi: use flexible array member for gmidi_in_port elements
  2016-01-18 16:02   ` Michal Nazarewicz
@ 2016-01-19  9:46     ` Felipe Ferreri Tonello
  0 siblings, 0 replies; 7+ messages in thread
From: Felipe Ferreri Tonello @ 2016-01-19  9:46 UTC (permalink / raw)
  To: Michal Nazarewicz, Felipe Balbi, Greg Kroah-Hartman,
	Dan Carpenter
  Cc: Robert Baldyga, Andrzej Pietrasiewicz, Pawel Szewczyk,
	linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1266 bytes --]

Hi Michal,

On 18/01/16 16:02, Michal Nazarewicz wrote:
>> On 09/01/16 03:47, Michal Nazarewicz wrote:
>>> @@ -55,7 +55,6 @@ static const char f_midi_longname[] = "MIDI Gadget";
>>>   * USB <- IN endpoint  <- rawmidi
>>>   */
>>>  struct gmidi_in_port {
>>> -	struct f_midi *midi;
> 
> On Wed, Jan 13 2016, Felipe Ferreri Tonello wrote:
>> This change is unrelated. I sent a patch removing this pointer as well.
> 
> Has it been merged yet?  Which branch?  Could you point me to the patch?

No, but it is in the mailing list.

"[PATCH 3/4] usb: gadget: f_midi: remove useless midi reference from
port struct" under "[PATCH 0/4] More improvements on MIDI gadget function"

> 
>>>  	int active;
>>>  	uint8_t cable;
>>>  	uint8_t state;
>>> @@ -1115,26 +1112,16 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
>>>  	}
>>>  
>>>  	/* allocate and initialize one new instance */
>>> -	midi = kzalloc(sizeof(*midi), GFP_KERNEL);
>>> +	midi = kzalloc(
>>> +		sizeof(*midi) + opts->in_ports * sizeof(*midi->in_ports_array),
>>> +		GFP_KERNEL);
>>
>> Is there a garantee that the compiler will always use in_ports_array at
>> the end of the allocated block?
> 
> Yep, see <https://en.wikipedia.org/wiki/Flexible_array_member>.

Thanks.

Felipe

[-- Attachment #2: 0x92698E6A.asc --]
[-- Type: application/pgp-keys, Size: 7195 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2016-01-19  9:45 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-01-09  3:47 [PATCHv2 1/2] usb: gadget: f_midi: use flexible array member for gmidi_in_port elements Michal Nazarewicz
2016-01-09  3:47 ` [PATCH 2/2] usb: gadget: f_midi: missing unlock on error path Michal Nazarewicz
2016-01-13 18:29   ` Felipe Ferreri Tonello
2016-01-13 18:26 ` [PATCHv2 1/2] usb: gadget: f_midi: use flexible array member for gmidi_in_port elements Felipe Ferreri Tonello
2016-01-13 20:41   ` Dan Carpenter
2016-01-18 16:02   ` Michal Nazarewicz
2016-01-19  9:46     ` Felipe Ferreri Tonello

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox