| Message ID | 26ee4adb-2637-52c3-ac83-ae121bed5eff@users.sourceforge.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
October 14, 2016 1:42 PM, "SF Markus Elfring" <elfring@users.sourceforge.net> wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Fri, 14 Oct 2016 07:34:46 +0200 > > Move the assignment for the local variable "data" behind the source code > for a memory allocation by this function. Sorry, I can't see what the point is? > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> > --- > drivers/media/rc/winbond-cir.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/rc/winbond-cir.c b/drivers/media/rc/winbond-cir.c > index 59050f5..fd997f0 100644 > --- a/drivers/media/rc/winbond-cir.c > +++ b/drivers/media/rc/winbond-cir.c > @@ -655,7 +655,7 @@ wbcir_txmask(struct rc_dev *dev, u32 mask) > static int > wbcir_tx(struct rc_dev *dev, unsigned *b, unsigned count) > { > - struct wbcir_data *data = dev->priv; > + struct wbcir_data *data; > unsigned *buf; > unsigned i; > unsigned long flags; > @@ -668,6 +668,7 @@ wbcir_tx(struct rc_dev *dev, unsigned *b, unsigned count) > for (i = 0; i < count; i++) > buf[i] = DIV_ROUND_CLOSEST(b[i], 10); > > + data = dev->priv; > /* Not sure if this is possible, but better safe than sorry */ > spin_lock_irqsave(&data->spinlock, flags); > if (data->txstate != WBCIR_TXSTATE_INACTIVE) { > -- > 2.10.1 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Move the assignment for the local variable "data" behind the source code >> for a memory allocation by this function. > > Sorry, I can't see what the point is? * How do you think about to avoid a variable assignment in case that this memory allocation failed anyhow? * Do you care for data access locality? Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
October 19, 2016 3:32 PM, "SF Markus Elfring" <elfring@users.sourceforge.net> wrote: >>> Move the assignment for the local variable "data" behind the source code >>> for a memory allocation by this function. >> >> Sorry, I can't see what the point is? > > * How do you think about to avoid a variable assignment in case > that this memory allocation failed anyhow? There is no memory allocation that can fail at this point. > * Do you care for data access locality? Not unless you can show measurable performance improvements? -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> * How do you think about to avoid a variable assignment in case >> that this memory allocation failed anyhow? > > There is no memory allocation that can fail at this point. Do you really know the failure probability for a call of the function "kmalloc" (within the function "wbcir_tx") under all possible run time situations? >> * Do you care for data access locality? > > Not unless you can show measurable performance improvements? Did any software developer (before me) dare anything in this direction? Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/rc/winbond-cir.c b/drivers/media/rc/winbond-cir.c index 59050f5..fd997f0 100644 --- a/drivers/media/rc/winbond-cir.c +++ b/drivers/media/rc/winbond-cir.c @@ -655,7 +655,7 @@ wbcir_txmask(struct rc_dev *dev, u32 mask) static int wbcir_tx(struct rc_dev *dev, unsigned *b, unsigned count) { - struct wbcir_data *data = dev->priv; + struct wbcir_data *data; unsigned *buf; unsigned i; unsigned long flags; @@ -668,6 +668,7 @@ wbcir_tx(struct rc_dev *dev, unsigned *b, unsigned count) for (i = 0; i < count; i++) buf[i] = DIV_ROUND_CLOSEST(b[i], 10); + data = dev->priv; /* Not sure if this is possible, but better safe than sorry */ spin_lock_irqsave(&data->spinlock, flags); if (data->txstate != WBCIR_TXSTATE_INACTIVE) {