Re: [PATCH] power: supply: lp8788: prevent out of bounds array access

[Giedrius Statkevičius]

Ping.

On Tue, Mar 28, 2017 at 2:23 AM, Kim, Milo <milo@ti.com> wrote:
> On 3/26/2017 1:00 AM, Giedrius Statkevičius wrote:
>>
>> val might become 7 in which case stime[7] (array of length 7) would be
>> accessed during the scnprintf call later and that will cause issues.
>> Obviously, string concatenation is not intended here so just a comma needs
>> to be added to fix the issue.
>>
>> Signed-off-by: Giedrius Statkevičius <giedrius.statkevic...@gmail.com>
>
>
> Acked-by: Milo Kim <milo@ti.com>
>
>
>> ---
>>  drivers/power/supply/lp8788-charger.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/power/supply/lp8788-charger.c
>> b/drivers/power/supply/lp8788-charger.c
>> index 509e2b341bd6..677f7c40b25a 100644
>> --- a/drivers/power/supply/lp8788-charger.c
>> +++ b/drivers/power/supply/lp8788-charger.c
>> @@ -651,7 +651,7 @@ static ssize_t lp8788_show_eoc_time(struct device
>> *dev,
>>  {
>> struct lp8788_charger *pchg = dev_get_drvdata(dev);
>> char *stime[] = { "400ms", "5min", "10min", "15min",
>> -   "20min", "25min", "30min" "No timeout" };
>> +   "20min", "25min", "30min", "No timeout" };
>> u8 val;
>>
>> lp8788_read_byte(pchg->lp, LP8788_CHG_EOC, );
>>
>

Re: [PATCH] power: supply: lp8788: prevent out of bounds array access

[Giedrius Statkevičius]

Ping.

On Tue, Mar 28, 2017 at 2:23 AM, Kim, Milo  wrote:
> On 3/26/2017 1:00 AM, Giedrius Statkevičius wrote:
>>
>> val might become 7 in which case stime[7] (array of length 7) would be
>> accessed during the scnprintf call later and that will cause issues.
>> Obviously, string concatenation is not intended here so just a comma needs
>> to be added to fix the issue.
>>
>> Signed-off-by: Giedrius Statkevičius 
>
>
> Acked-by: Milo Kim 
>
>
>> ---
>>  drivers/power/supply/lp8788-charger.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/power/supply/lp8788-charger.c
>> b/drivers/power/supply/lp8788-charger.c
>> index 509e2b341bd6..677f7c40b25a 100644
>> --- a/drivers/power/supply/lp8788-charger.c
>> +++ b/drivers/power/supply/lp8788-charger.c
>> @@ -651,7 +651,7 @@ static ssize_t lp8788_show_eoc_time(struct device
>> *dev,
>>  {
>> struct lp8788_charger *pchg = dev_get_drvdata(dev);
>> char *stime[] = { "400ms", "5min", "10min", "15min",
>> -   "20min", "25min", "30min" "No timeout" };
>> +   "20min", "25min", "30min", "No timeout" };
>> u8 val;
>>
>> lp8788_read_byte(pchg->lp, LP8788_CHG_EOC, );
>>
>

[PATCH] power: supply: lp8788: prevent out of bounds array access

[Giedrius Statkevičius]

val might become 7 in which case stime[7] (array of length 7) would be
accessed during the scnprintf call later and that will cause issues.
Obviously, string concatenation is not intended here so just a comma needs
to be added to fix the issue.

Signed-off-by: Giedrius Statkevičius <giedrius.statkevic...@gmail.com>
---
 drivers/power/supply/lp8788-charger.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/power/supply/lp8788-charger.c 
b/drivers/power/supply/lp8788-charger.c
index 509e2b341bd6..677f7c40b25a 100644
--- a/drivers/power/supply/lp8788-charger.c
+++ b/drivers/power/supply/lp8788-charger.c
@@ -651,7 +651,7 @@ static ssize_t lp8788_show_eoc_time(struct device *dev,
 {
struct lp8788_charger *pchg = dev_get_drvdata(dev);
char *stime[] = { "400ms", "5min", "10min", "15min",
-   "20min", "25min", "30min" "No timeout" };
+   "20min", "25min", "30min", "No timeout" };
u8 val;
 
lp8788_read_byte(pchg->lp, LP8788_CHG_EOC, );
-- 
2.12.0

[PATCH] power: supply: lp8788: prevent out of bounds array access

[Giedrius Statkevičius]

val might become 7 in which case stime[7] (array of length 7) would be
accessed during the scnprintf call later and that will cause issues.
Obviously, string concatenation is not intended here so just a comma needs
to be added to fix the issue.

Signed-off-by: Giedrius Statkevičius 
---
 drivers/power/supply/lp8788-charger.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/power/supply/lp8788-charger.c 
b/drivers/power/supply/lp8788-charger.c
index 509e2b341bd6..677f7c40b25a 100644
--- a/drivers/power/supply/lp8788-charger.c
+++ b/drivers/power/supply/lp8788-charger.c
@@ -651,7 +651,7 @@ static ssize_t lp8788_show_eoc_time(struct device *dev,
 {
struct lp8788_charger *pchg = dev_get_drvdata(dev);
char *stime[] = { "400ms", "5min", "10min", "15min",
-   "20min", "25min", "30min" "No timeout" };
+   "20min", "25min", "30min", "No timeout" };
u8 val;
 
lp8788_read_byte(pchg->lp, LP8788_CHG_EOC, );
-- 
2.12.0

Re: [PATCH 00/87] Fix some style warnings in hfa384x.h

[Giedrius Statkevičius]

On Wed, Sep 28, 2016 at 7:27 PM, Sergio Paracuellos
 wrote:
> This patch series fix some warnings reported by checkpatch.pl script in 
> hfa384x.h:
> WARNING: Block comments use * on subsequent lines
> WARNING: Block comments use a trailing */ on a separate line
> WARNING: do not add new typedefs
>
> Sergio Paracuellos (87):
[]

Why some of them (if not all) have duplicated Signed-off-by lines?

Re: [PATCH 00/87] Fix some style warnings in hfa384x.h

[Giedrius Statkevičius]

On Wed, Sep 28, 2016 at 7:27 PM, Sergio Paracuellos
 wrote:
> This patch series fix some warnings reported by checkpatch.pl script in 
> hfa384x.h:
> WARNING: Block comments use * on subsequent lines
> WARNING: Block comments use a trailing */ on a separate line
> WARNING: do not add new typedefs
>
> Sergio Paracuellos (87):
[]

Why some of them (if not all) have duplicated Signed-off-by lines?

Re: [PATCH] Staging: comedi: ni_daq_dio24.c: Fix block comments use * on subsequent lines.

[Giedrius Statkevičius]

On Sun, Sep 4, 2016 at 12:08 PM, Amit Ghadge  wrote:
> This is a patch to the ni_daq_dio24.c that fixes checkpatch warning:
> WARNING: Block comments use * on subsequent lines
>
> Signed-off-by: Amit Ghadge 
> ---
[...]

Why are you sending so many copies of the same patch?

Re: [PATCH] Staging: comedi: ni_daq_dio24.c: Fix block comments use * on subsequent lines.

[Giedrius Statkevičius]

On Sun, Sep 4, 2016 at 12:08 PM, Amit Ghadge  wrote:
> This is a patch to the ni_daq_dio24.c that fixes checkpatch warning:
> WARNING: Block comments use * on subsequent lines
>
> Signed-off-by: Amit Ghadge 
> ---
[...]

Why are you sending so many copies of the same patch?

Re: [PATCH] ath9k: bring back direction setting in ath9k_{start_stop}

[Giedrius Statkevičius]

Some more users complaining about this:
https://bbs.archlinux.org/viewtopic.php?id=215978

On Thu, Sep 01, 2016 at 08:47:02PM +0300, Giedrius Statkevičius wrote:
> A regression was introduced in commit id 79d4db1214a ("ath9k: cleanup
> led_pin initial") that broken the WLAN status led on my laptop with
> AR9287 after suspending and resuming.
> 
> Steps to reproduce:
> * Suspend (laptop)
> * Resume (laptop)
> * Observe that the WLAN led no longer turns ON/OFF depending on the
>   status and is always red
> 
> Even though for my case it only needs to be set to OUT in ath9k_start
> but for consistency bring back the IN direction setting as well.
> 
> Cc: Miaoqing Pan <miaoq...@codeaurora.org>
> Cc: Kalle Valo <kv...@qca.qualcomm.com>
> Cc: <sta...@vger.kernel.org>
> Signed-off-by: Giedrius Statkevičius <giedrius.statkevic...@gmail.com>
> ---
[...]

Re: [PATCH] ath9k: bring back direction setting in ath9k_{start_stop}

[Giedrius Statkevičius]

Some more users complaining about this:
https://bbs.archlinux.org/viewtopic.php?id=215978

On Thu, Sep 01, 2016 at 08:47:02PM +0300, Giedrius Statkevičius wrote:
> A regression was introduced in commit id 79d4db1214a ("ath9k: cleanup
> led_pin initial") that broken the WLAN status led on my laptop with
> AR9287 after suspending and resuming.
> 
> Steps to reproduce:
> * Suspend (laptop)
> * Resume (laptop)
> * Observe that the WLAN led no longer turns ON/OFF depending on the
>   status and is always red
> 
> Even though for my case it only needs to be set to OUT in ath9k_start
> but for consistency bring back the IN direction setting as well.
> 
> Cc: Miaoqing Pan 
> Cc: Kalle Valo 
> Cc: 
> Signed-off-by: Giedrius Statkevičius 
> ---
[...]

Re: [PATCH] asus-laptop: get rid of parse_arg()

[Giedrius Statkevičius]

On Wed, Aug 17, 2016 at 11:23:15AM -0700, Darren Hart wrote:
> On Tue, Aug 16, 2016 at 12:49:50PM +0300, Giedrius Statkevičius wrote:
> > On Fri, Aug 12, 2016 at 02:40:02PM -0700, Darren Hart wrote:
> > > On Sat, Aug 06, 2016 at 08:00:26PM +0300, Giedrius Statkevičius wrote:
> > > > On Fri, Aug 05, 2016 at 04:15:07PM -0700, Darren Hart wrote:
> > > > > On Fri, Aug 05, 2016 at 11:57:10PM +0300, Giedrius Statkevičius wrote:
> > > > > > parse_arg() duplicates the funcionality of kstrtoint() so use the 
> > > > > > latter
> > > > > > function instead. There is no funcionality change except that in the
> > > > > > case of input being too big -ERANGE will be returned instead of 
> > > > > > -EINVAL
> > > > > > which is not bad because -ERANGE makes more sense here. The check 
> > > > > > for
> > > > > > !count is already done by the sysfs core so no need to duplicate it
> > > > > > again. Also, add some minor corrections to error handling to 
> > > > > > accommodate
> > > > > > the change in return values (parse_arg returned count if everything
> > > > > > succeeded whereas kstrtoint returns 0 in the same situation)
> > > > > > 
> > > > > > As a result of this patch asus-laptop.ko size is reduced by almost 
> > > > > > 1%:
> > > > > > add/remove: 0/1 grow/shrink: 1/6 up/down: 1/-149 (-148)
> > > > > > function old new   delta
> > > > > > __UNIQUE_ID_vermagic0 69  70  +1
> > > > > > ls_switch_store  133 117 -16
> > > > > > ledd_store   175 159 -16
> > > > > > display_store157 141 -16
> > > > > > ls_level_store   193 176 -17
> > > > > > gps_store200 178 -22
> > > > > > sysfs_acpi_set.isra  148 125 -23
> > > > > > parse_arg.part39   - -39
> > > > > > Total: Before=19160, After=19012, chg -0.77%
> > > > > > 
> > > > > > Signed-off-by: Giedrius Statkevičius 
> > > > > > <giedrius.statkevic...@gmail.com>
> > > > > > ---
> > > > > >  drivers/platform/x86/asus-laptop.c | 77 
> > > > > > ++
> > > > > >  1 file changed, 36 insertions(+), 41 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/platform/x86/asus-laptop.c 
> > > > > > b/drivers/platform/x86/asus-laptop.c
> > > > > > index 15f1311..28551f5 100644
> > > > > > --- a/drivers/platform/x86/asus-laptop.c
> > > > > > +++ b/drivers/platform/x86/asus-laptop.c
> > > > > > @@ -932,30 +932,19 @@ static ssize_t infos_show(struct device *dev, 
> > > > > > struct device_attribute *attr,
> > > > > >  }
> > > > > >  static DEVICE_ATTR_RO(infos);
> > > > > >  
> > > > > > -static int parse_arg(const char *buf, unsigned long count, int 
> > > > > > *val)
> > > > > > -{
> > > > > > -   if (!count)
> > > > > > -   return 0;
> > > > > > -   if (count > 31)
> > > > > > -   return -EINVAL;
> > > > > > -   if (sscanf(buf, "%i", val) != 1)
> > > > > > -   return -EINVAL;
> > > > > > -   return count;
> > > > > > -}
> > > > > > -
> > > > > >  static ssize_t sysfs_acpi_set(struct asus_laptop *asus,
> > > > > >   const char *buf, size_t count,
> > > > > >   const char *method)
> > > > > >  {
> > > > > > int rv, value;
> > > > > >  
> > > > > > -   rv = parse_arg(buf, count, );
> > > > > > -   if (rv <= 0)
> > > > > > +   rv = kstrtoint(buf, 0, );
> > > > > > +   if (rv < 0)
> > > > > > return rv;
> > > > > >  
> > > > > > if (write_a

Re: [PATCH] asus-laptop: get rid of parse_arg()

[Giedrius Statkevičius]

On Wed, Aug 17, 2016 at 11:23:15AM -0700, Darren Hart wrote:
> On Tue, Aug 16, 2016 at 12:49:50PM +0300, Giedrius Statkevičius wrote:
> > On Fri, Aug 12, 2016 at 02:40:02PM -0700, Darren Hart wrote:
> > > On Sat, Aug 06, 2016 at 08:00:26PM +0300, Giedrius Statkevičius wrote:
> > > > On Fri, Aug 05, 2016 at 04:15:07PM -0700, Darren Hart wrote:
> > > > > On Fri, Aug 05, 2016 at 11:57:10PM +0300, Giedrius Statkevičius wrote:
> > > > > > parse_arg() duplicates the funcionality of kstrtoint() so use the 
> > > > > > latter
> > > > > > function instead. There is no funcionality change except that in the
> > > > > > case of input being too big -ERANGE will be returned instead of 
> > > > > > -EINVAL
> > > > > > which is not bad because -ERANGE makes more sense here. The check 
> > > > > > for
> > > > > > !count is already done by the sysfs core so no need to duplicate it
> > > > > > again. Also, add some minor corrections to error handling to 
> > > > > > accommodate
> > > > > > the change in return values (parse_arg returned count if everything
> > > > > > succeeded whereas kstrtoint returns 0 in the same situation)
> > > > > > 
> > > > > > As a result of this patch asus-laptop.ko size is reduced by almost 
> > > > > > 1%:
> > > > > > add/remove: 0/1 grow/shrink: 1/6 up/down: 1/-149 (-148)
> > > > > > function old new   delta
> > > > > > __UNIQUE_ID_vermagic0 69  70  +1
> > > > > > ls_switch_store  133 117 -16
> > > > > > ledd_store   175 159 -16
> > > > > > display_store157 141 -16
> > > > > > ls_level_store   193 176 -17
> > > > > > gps_store200 178 -22
> > > > > > sysfs_acpi_set.isra  148 125 -23
> > > > > > parse_arg.part39   - -39
> > > > > > Total: Before=19160, After=19012, chg -0.77%
> > > > > > 
> > > > > > Signed-off-by: Giedrius Statkevičius 
> > > > > > 
> > > > > > ---
> > > > > >  drivers/platform/x86/asus-laptop.c | 77 
> > > > > > ++
> > > > > >  1 file changed, 36 insertions(+), 41 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/platform/x86/asus-laptop.c 
> > > > > > b/drivers/platform/x86/asus-laptop.c
> > > > > > index 15f1311..28551f5 100644
> > > > > > --- a/drivers/platform/x86/asus-laptop.c
> > > > > > +++ b/drivers/platform/x86/asus-laptop.c
> > > > > > @@ -932,30 +932,19 @@ static ssize_t infos_show(struct device *dev, 
> > > > > > struct device_attribute *attr,
> > > > > >  }
> > > > > >  static DEVICE_ATTR_RO(infos);
> > > > > >  
> > > > > > -static int parse_arg(const char *buf, unsigned long count, int 
> > > > > > *val)
> > > > > > -{
> > > > > > -   if (!count)
> > > > > > -   return 0;
> > > > > > -   if (count > 31)
> > > > > > -   return -EINVAL;
> > > > > > -   if (sscanf(buf, "%i", val) != 1)
> > > > > > -   return -EINVAL;
> > > > > > -   return count;
> > > > > > -}
> > > > > > -
> > > > > >  static ssize_t sysfs_acpi_set(struct asus_laptop *asus,
> > > > > >   const char *buf, size_t count,
> > > > > >   const char *method)
> > > > > >  {
> > > > > > int rv, value;
> > > > > >  
> > > > > > -   rv = parse_arg(buf, count, );
> > > > > > -   if (rv <= 0)
> > > > > > +   rv = kstrtoint(buf, 0, );
> > > > > > +   if (rv < 0)
> > > > > > return rv;
> > > > > >  
> > > > > > if (write_acpi_int(asus->handle, method, value

[PATCH] ath9k: bring back direction setting in ath9k_{start_stop}

[Giedrius Statkevičius]

A regression was introduced in commit id 79d4db1214a ("ath9k: cleanup
led_pin initial") that broken the WLAN status led on my laptop with
AR9287 after suspending and resuming.

Steps to reproduce:
* Suspend (laptop)
* Resume (laptop)
* Observe that the WLAN led no longer turns ON/OFF depending on the
  status and is always red

Even though for my case it only needs to be set to OUT in ath9k_start
but for consistency bring back the IN direction setting as well.

Cc: Miaoqing Pan <miaoq...@codeaurora.org>
Cc: Kalle Valo <kv...@qca.qualcomm.com>
Cc: <sta...@vger.kernel.org>
Signed-off-by: Giedrius Statkevičius <giedrius.statkevic...@gmail.com>
---
This patch should be applied to all 4.7 and later kernels

Another user complaining about probably the same problem:
https://bugzilla.kernel.org/show_bug.cgi?id=151711

 drivers/net/wireless/ath/ath9k/main.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/main.c 
b/drivers/net/wireless/ath/ath9k/main.c
index 8b63988..121dc05 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -718,9 +718,12 @@ static int ath9k_start(struct ieee80211_hw *hw)
if (!ath_complete_reset(sc, false))
ah->reset_power_on = false;
 
-   if (ah->led_pin >= 0)
+   if (ah->led_pin >= 0) {
ath9k_hw_set_gpio(ah, ah->led_pin,
  (ah->config.led_active_high) ? 1 : 0);
+   ath9k_hw_gpio_request_out(ah, ah->led_pin, NULL,
+ AR_GPIO_OUTPUT_MUX_AS_OUTPUT);
+   }
 
/*
 * Reset key cache to sane defaults (all entries cleared) instead of
@@ -864,9 +867,11 @@ static void ath9k_stop(struct ieee80211_hw *hw)
 
spin_lock_bh(>sc_pcu_lock);
 
-   if (ah->led_pin >= 0)
+   if (ah->led_pin >= 0) {
ath9k_hw_set_gpio(ah, ah->led_pin,
  (ah->config.led_active_high) ? 0 : 1);
+   ath9k_hw_gpio_request_in(ah, ah->led_pin, NULL);
+   }
 
ath_prepare_reset(sc);
 
-- 
2.9.3

[PATCH] ath9k: bring back direction setting in ath9k_{start_stop}

[Giedrius Statkevičius]

A regression was introduced in commit id 79d4db1214a ("ath9k: cleanup
led_pin initial") that broken the WLAN status led on my laptop with
AR9287 after suspending and resuming.

Steps to reproduce:
* Suspend (laptop)
* Resume (laptop)
* Observe that the WLAN led no longer turns ON/OFF depending on the
  status and is always red

Even though for my case it only needs to be set to OUT in ath9k_start
but for consistency bring back the IN direction setting as well.

Cc: Miaoqing Pan 
Cc: Kalle Valo 
Cc: 
Signed-off-by: Giedrius Statkevičius 
---
This patch should be applied to all 4.7 and later kernels

Another user complaining about probably the same problem:
https://bugzilla.kernel.org/show_bug.cgi?id=151711

 drivers/net/wireless/ath/ath9k/main.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/main.c 
b/drivers/net/wireless/ath/ath9k/main.c
index 8b63988..121dc05 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -718,9 +718,12 @@ static int ath9k_start(struct ieee80211_hw *hw)
if (!ath_complete_reset(sc, false))
ah->reset_power_on = false;
 
-   if (ah->led_pin >= 0)
+   if (ah->led_pin >= 0) {
ath9k_hw_set_gpio(ah, ah->led_pin,
  (ah->config.led_active_high) ? 1 : 0);
+   ath9k_hw_gpio_request_out(ah, ah->led_pin, NULL,
+ AR_GPIO_OUTPUT_MUX_AS_OUTPUT);
+   }
 
/*
 * Reset key cache to sane defaults (all entries cleared) instead of
@@ -864,9 +867,11 @@ static void ath9k_stop(struct ieee80211_hw *hw)
 
spin_lock_bh(>sc_pcu_lock);
 
-   if (ah->led_pin >= 0)
+   if (ah->led_pin >= 0) {
ath9k_hw_set_gpio(ah, ah->led_pin,
  (ah->config.led_active_high) ? 0 : 1);
+   ath9k_hw_gpio_request_in(ah, ah->led_pin, NULL);
+   }
 
ath_prepare_reset(sc);
 
-- 
2.9.3

Re: [PATCH] asus-laptop: get rid of parse_arg()

[Giedrius Statkevičius]

On Fri, Aug 12, 2016 at 02:40:02PM -0700, Darren Hart wrote:
> On Sat, Aug 06, 2016 at 08:00:26PM +0300, Giedrius Statkevičius wrote:
> > On Fri, Aug 05, 2016 at 04:15:07PM -0700, Darren Hart wrote:
> > > On Fri, Aug 05, 2016 at 11:57:10PM +0300, Giedrius Statkevičius wrote:
> > > > parse_arg() duplicates the funcionality of kstrtoint() so use the latter
> > > > function instead. There is no funcionality change except that in the
> > > > case of input being too big -ERANGE will be returned instead of -EINVAL
> > > > which is not bad because -ERANGE makes more sense here. The check for
> > > > !count is already done by the sysfs core so no need to duplicate it
> > > > again. Also, add some minor corrections to error handling to accommodate
> > > > the change in return values (parse_arg returned count if everything
> > > > succeeded whereas kstrtoint returns 0 in the same situation)
> > > > 
> > > > As a result of this patch asus-laptop.ko size is reduced by almost 1%:
> > > > add/remove: 0/1 grow/shrink: 1/6 up/down: 1/-149 (-148)
> > > > function old new   delta
> > > > __UNIQUE_ID_vermagic0 69  70  +1
> > > > ls_switch_store  133 117 -16
> > > > ledd_store   175 159 -16
> > > > display_store157 141 -16
> > > > ls_level_store   193 176 -17
> > > > gps_store200 178 -22
> > > > sysfs_acpi_set.isra  148 125 -23
> > > > parse_arg.part39   - -39
> > > > Total: Before=19160, After=19012, chg -0.77%
> > > > 
> > > > Signed-off-by: Giedrius Statkevičius <giedrius.statkevic...@gmail.com>
> > > > ---
> > > >  drivers/platform/x86/asus-laptop.c | 77 
> > > > ++
> > > >  1 file changed, 36 insertions(+), 41 deletions(-)
> > > > 
> > > > diff --git a/drivers/platform/x86/asus-laptop.c 
> > > > b/drivers/platform/x86/asus-laptop.c
> > > > index 15f1311..28551f5 100644
> > > > --- a/drivers/platform/x86/asus-laptop.c
> > > > +++ b/drivers/platform/x86/asus-laptop.c
> > > > @@ -932,30 +932,19 @@ static ssize_t infos_show(struct device *dev, 
> > > > struct device_attribute *attr,
> > > >  }
> > > >  static DEVICE_ATTR_RO(infos);
> > > >  
> > > > -static int parse_arg(const char *buf, unsigned long count, int *val)
> > > > -{
> > > > -   if (!count)
> > > > -   return 0;
> > > > -   if (count > 31)
> > > > -   return -EINVAL;
> > > > -   if (sscanf(buf, "%i", val) != 1)
> > > > -   return -EINVAL;
> > > > -   return count;
> > > > -}
> > > > -
> > > >  static ssize_t sysfs_acpi_set(struct asus_laptop *asus,
> > > >   const char *buf, size_t count,
> > > >   const char *method)
> > > >  {
> > > > int rv, value;
> > > >  
> > > > -   rv = parse_arg(buf, count, );
> > > > -   if (rv <= 0)
> > > > +   rv = kstrtoint(buf, 0, );
> > > > +   if (rv < 0)
> > > > return rv;
> > > >  
> > > > if (write_acpi_int(asus->handle, method, value))
> > > > return -ENODEV;
> > > > -   return rv;
> > > > +   return count;
> > > 
> > > This makes explicit what was hidden before - count is merely a range 
> > > check, it
> > > isn't used in parsing the string... I'm not sure if this is a problem, 
> > > but it
> > > caught my interest. If count is passed as 12, but buf only contains 3 
> > > character,
> > > it may succeed and return 12. I suppose this is a failure in the caller, 
> > > and
> > > doesn't impact this function - unless the caller isn't expected to 
> > > properly
> > > terminate the string... but if that were the case, it would have failed
> > > previously as we didn't check for that in parse_arg either this is 
>

Re: [PATCH] asus-laptop: get rid of parse_arg()

[Giedrius Statkevičius]

On Fri, Aug 12, 2016 at 02:40:02PM -0700, Darren Hart wrote:
> On Sat, Aug 06, 2016 at 08:00:26PM +0300, Giedrius Statkevičius wrote:
> > On Fri, Aug 05, 2016 at 04:15:07PM -0700, Darren Hart wrote:
> > > On Fri, Aug 05, 2016 at 11:57:10PM +0300, Giedrius Statkevičius wrote:
> > > > parse_arg() duplicates the funcionality of kstrtoint() so use the latter
> > > > function instead. There is no funcionality change except that in the
> > > > case of input being too big -ERANGE will be returned instead of -EINVAL
> > > > which is not bad because -ERANGE makes more sense here. The check for
> > > > !count is already done by the sysfs core so no need to duplicate it
> > > > again. Also, add some minor corrections to error handling to accommodate
> > > > the change in return values (parse_arg returned count if everything
> > > > succeeded whereas kstrtoint returns 0 in the same situation)
> > > > 
> > > > As a result of this patch asus-laptop.ko size is reduced by almost 1%:
> > > > add/remove: 0/1 grow/shrink: 1/6 up/down: 1/-149 (-148)
> > > > function old new   delta
> > > > __UNIQUE_ID_vermagic0 69  70  +1
> > > > ls_switch_store  133 117 -16
> > > > ledd_store   175 159 -16
> > > > display_store157 141 -16
> > > > ls_level_store   193 176 -17
> > > > gps_store200 178 -22
> > > > sysfs_acpi_set.isra  148 125 -23
> > > > parse_arg.part39   - -39
> > > > Total: Before=19160, After=19012, chg -0.77%
> > > > 
> > > > Signed-off-by: Giedrius Statkevičius 
> > > > ---
> > > >  drivers/platform/x86/asus-laptop.c | 77 
> > > > ++
> > > >  1 file changed, 36 insertions(+), 41 deletions(-)
> > > > 
> > > > diff --git a/drivers/platform/x86/asus-laptop.c 
> > > > b/drivers/platform/x86/asus-laptop.c
> > > > index 15f1311..28551f5 100644
> > > > --- a/drivers/platform/x86/asus-laptop.c
> > > > +++ b/drivers/platform/x86/asus-laptop.c
> > > > @@ -932,30 +932,19 @@ static ssize_t infos_show(struct device *dev, 
> > > > struct device_attribute *attr,
> > > >  }
> > > >  static DEVICE_ATTR_RO(infos);
> > > >  
> > > > -static int parse_arg(const char *buf, unsigned long count, int *val)
> > > > -{
> > > > -   if (!count)
> > > > -   return 0;
> > > > -   if (count > 31)
> > > > -   return -EINVAL;
> > > > -   if (sscanf(buf, "%i", val) != 1)
> > > > -   return -EINVAL;
> > > > -   return count;
> > > > -}
> > > > -
> > > >  static ssize_t sysfs_acpi_set(struct asus_laptop *asus,
> > > >   const char *buf, size_t count,
> > > >   const char *method)
> > > >  {
> > > > int rv, value;
> > > >  
> > > > -   rv = parse_arg(buf, count, );
> > > > -   if (rv <= 0)
> > > > +   rv = kstrtoint(buf, 0, );
> > > > +   if (rv < 0)
> > > > return rv;
> > > >  
> > > > if (write_acpi_int(asus->handle, method, value))
> > > > return -ENODEV;
> > > > -   return rv;
> > > > +   return count;
> > > 
> > > This makes explicit what was hidden before - count is merely a range 
> > > check, it
> > > isn't used in parsing the string... I'm not sure if this is a problem, 
> > > but it
> > > caught my interest. If count is passed as 12, but buf only contains 3 
> > > character,
> > > it may succeed and return 12. I suppose this is a failure in the caller, 
> > > and
> > > doesn't impact this function - unless the caller isn't expected to 
> > > properly
> > > terminate the string... but if that were the case, it would have failed
> > > previously as we didn't check for that in parse_arg either this is 
> > > fine as
> > > is I suppose - can be addressed separately 

Re: [PATCH] asus-laptop: get rid of parse_arg()

[Giedrius Statkevičius]

On Fri, Aug 05, 2016 at 04:15:07PM -0700, Darren Hart wrote:
> On Fri, Aug 05, 2016 at 11:57:10PM +0300, Giedrius Statkevičius wrote:
> > parse_arg() duplicates the funcionality of kstrtoint() so use the latter
> > function instead. There is no funcionality change except that in the
> > case of input being too big -ERANGE will be returned instead of -EINVAL
> > which is not bad because -ERANGE makes more sense here. The check for
> > !count is already done by the sysfs core so no need to duplicate it
> > again. Also, add some minor corrections to error handling to accommodate
> > the change in return values (parse_arg returned count if everything
> > succeeded whereas kstrtoint returns 0 in the same situation)
> > 
> > As a result of this patch asus-laptop.ko size is reduced by almost 1%:
> > add/remove: 0/1 grow/shrink: 1/6 up/down: 1/-149 (-148)
> > function old new   delta
> > __UNIQUE_ID_vermagic0 69  70  +1
> > ls_switch_store  133 117 -16
> > ledd_store   175 159 -16
> > display_store157 141 -16
> > ls_level_store   193 176 -17
> > gps_store200 178 -22
> > sysfs_acpi_set.isra  148 125 -23
> > parse_arg.part    39   - -39
> > Total: Before=19160, After=19012, chg -0.77%
> > 
> > Signed-off-by: Giedrius Statkevičius <giedrius.statkevic...@gmail.com>
> > ---
> >  drivers/platform/x86/asus-laptop.c | 77 
> > ++
> >  1 file changed, 36 insertions(+), 41 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/asus-laptop.c 
> > b/drivers/platform/x86/asus-laptop.c
> > index 15f1311..28551f5 100644
> > --- a/drivers/platform/x86/asus-laptop.c
> > +++ b/drivers/platform/x86/asus-laptop.c
> > @@ -932,30 +932,19 @@ static ssize_t infos_show(struct device *dev, struct 
> > device_attribute *attr,
> >  }
> >  static DEVICE_ATTR_RO(infos);
> >  
> > -static int parse_arg(const char *buf, unsigned long count, int *val)
> > -{
> > -   if (!count)
> > -   return 0;
> > -   if (count > 31)
> > -   return -EINVAL;
> > -   if (sscanf(buf, "%i", val) != 1)
> > -   return -EINVAL;
> > -   return count;
> > -}
> > -
> >  static ssize_t sysfs_acpi_set(struct asus_laptop *asus,
> >   const char *buf, size_t count,
> >   const char *method)
> >  {
> > int rv, value;
> >  
> > -   rv = parse_arg(buf, count, );
> > -   if (rv <= 0)
> > +   rv = kstrtoint(buf, 0, );
> > +   if (rv < 0)
> > return rv;
> >  
> > if (write_acpi_int(asus->handle, method, value))
> > return -ENODEV;
> > -   return rv;
> > +   return count;
> 
> This makes explicit what was hidden before - count is merely a range check, it
> isn't used in parsing the string... I'm not sure if this is a problem, but it
> caught my interest. If count is passed as 12, but buf only contains 3 
> character,
> it may succeed and return 12. I suppose this is a failure in the caller, and
> doesn't impact this function - unless the caller isn't expected to properly
> terminate the string... but if that were the case, it would have failed
> previously as we didn't check for that in parse_arg either this is fine as
> is I suppose - can be addressed separately if need be.
> 
According to Documentation/filesystems/sysfs.txt:
"On write(2), ... A terminating null is added after the data on stores. This
makes functions like sysfs_streq() safe to use."
So it should be guaranteed that the buffer is a proper C string. Also, we could
say kstrtoint() or sscanf() uses all of the buffer so it is safe to return count
(as it says in the documentation) as it was before this patch (parse_int
returned count if everything succeeded).

> >  }
> >  
> >  /*
> > @@ -975,15 +964,17 @@ static ssize_t ledd_store(struct device *dev, struct 
> > device_attribute *attr,
> > struct asus_laptop *asus = dev_get_drvdata(dev);
> > int rv, value;
> >  
> > -   rv = parse_arg(buf, count, );
> > -   if (rv > 0) {
> > -   if (write_acpi_int(asus->handle, METHOD_LEDD, value)) {
> > -   pr_warn("LED display write failed\n");
> > -   return -ENODEV;

Re: [PATCH] asus-laptop: get rid of parse_arg()

[Giedrius Statkevičius]

On Fri, Aug 05, 2016 at 04:15:07PM -0700, Darren Hart wrote:
> On Fri, Aug 05, 2016 at 11:57:10PM +0300, Giedrius Statkevičius wrote:
> > parse_arg() duplicates the funcionality of kstrtoint() so use the latter
> > function instead. There is no funcionality change except that in the
> > case of input being too big -ERANGE will be returned instead of -EINVAL
> > which is not bad because -ERANGE makes more sense here. The check for
> > !count is already done by the sysfs core so no need to duplicate it
> > again. Also, add some minor corrections to error handling to accommodate
> > the change in return values (parse_arg returned count if everything
> > succeeded whereas kstrtoint returns 0 in the same situation)
> > 
> > As a result of this patch asus-laptop.ko size is reduced by almost 1%:
> > add/remove: 0/1 grow/shrink: 1/6 up/down: 1/-149 (-148)
> > function old new   delta
> > __UNIQUE_ID_vermagic0 69  70  +1
> > ls_switch_store  133 117 -16
> > ledd_store   175 159 -16
> > display_store157 141 -16
> > ls_level_store   193 176 -17
> > gps_store200 178 -22
> > sysfs_acpi_set.isra  148 125 -23
> > parse_arg.part    39   - -39
> > Total: Before=19160, After=19012, chg -0.77%
> > 
> > Signed-off-by: Giedrius Statkevičius 
> > ---
> >  drivers/platform/x86/asus-laptop.c | 77 
> > ++
> >  1 file changed, 36 insertions(+), 41 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/asus-laptop.c 
> > b/drivers/platform/x86/asus-laptop.c
> > index 15f1311..28551f5 100644
> > --- a/drivers/platform/x86/asus-laptop.c
> > +++ b/drivers/platform/x86/asus-laptop.c
> > @@ -932,30 +932,19 @@ static ssize_t infos_show(struct device *dev, struct 
> > device_attribute *attr,
> >  }
> >  static DEVICE_ATTR_RO(infos);
> >  
> > -static int parse_arg(const char *buf, unsigned long count, int *val)
> > -{
> > -   if (!count)
> > -   return 0;
> > -   if (count > 31)
> > -   return -EINVAL;
> > -   if (sscanf(buf, "%i", val) != 1)
> > -   return -EINVAL;
> > -   return count;
> > -}
> > -
> >  static ssize_t sysfs_acpi_set(struct asus_laptop *asus,
> >   const char *buf, size_t count,
> >   const char *method)
> >  {
> > int rv, value;
> >  
> > -   rv = parse_arg(buf, count, );
> > -   if (rv <= 0)
> > +   rv = kstrtoint(buf, 0, );
> > +   if (rv < 0)
> > return rv;
> >  
> > if (write_acpi_int(asus->handle, method, value))
> > return -ENODEV;
> > -   return rv;
> > +   return count;
> 
> This makes explicit what was hidden before - count is merely a range check, it
> isn't used in parsing the string... I'm not sure if this is a problem, but it
> caught my interest. If count is passed as 12, but buf only contains 3 
> character,
> it may succeed and return 12. I suppose this is a failure in the caller, and
> doesn't impact this function - unless the caller isn't expected to properly
> terminate the string... but if that were the case, it would have failed
> previously as we didn't check for that in parse_arg either this is fine as
> is I suppose - can be addressed separately if need be.
> 
According to Documentation/filesystems/sysfs.txt:
"On write(2), ... A terminating null is added after the data on stores. This
makes functions like sysfs_streq() safe to use."
So it should be guaranteed that the buffer is a proper C string. Also, we could
say kstrtoint() or sscanf() uses all of the buffer so it is safe to return count
(as it says in the documentation) as it was before this patch (parse_int
returned count if everything succeeded).

> >  }
> >  
> >  /*
> > @@ -975,15 +964,17 @@ static ssize_t ledd_store(struct device *dev, struct 
> > device_attribute *attr,
> > struct asus_laptop *asus = dev_get_drvdata(dev);
> > int rv, value;
> >  
> > -   rv = parse_arg(buf, count, );
> > -   if (rv > 0) {
> > -   if (write_acpi_int(asus->handle, METHOD_LEDD, value)) {
> > -   pr_warn("LED display write failed\n");
> > -   return -ENODEV;
> > -   }
> > -

[PATCH] asus-laptop: get rid of parse_arg()

[Giedrius Statkevičius]

parse_arg() duplicates the funcionality of kstrtoint() so use the latter
function instead. There is no funcionality change except that in the
case of input being too big -ERANGE will be returned instead of -EINVAL
which is not bad because -ERANGE makes more sense here. The check for
!count is already done by the sysfs core so no need to duplicate it
again. Also, add some minor corrections to error handling to accommodate
the change in return values (parse_arg returned count if everything
succeeded whereas kstrtoint returns 0 in the same situation)

As a result of this patch asus-laptop.ko size is reduced by almost 1%:
add/remove: 0/1 grow/shrink: 1/6 up/down: 1/-149 (-148)
function old new   delta
__UNIQUE_ID_vermagic0 69  70  +1
ls_switch_store  133 117 -16
ledd_store   175 159 -16
display_store157 141 -16
ls_level_store   193 176 -17
gps_store200 178 -22
sysfs_acpi_set.isra  148 125 -23
parse_arg.part39   - -39
Total: Before=19160, After=19012, chg -0.77%

Signed-off-by: Giedrius Statkevičius <giedrius.statkevic...@gmail.com>
---
 drivers/platform/x86/asus-laptop.c | 77 ++
 1 file changed, 36 insertions(+), 41 deletions(-)

diff --git a/drivers/platform/x86/asus-laptop.c 
b/drivers/platform/x86/asus-laptop.c
index 15f1311..28551f5 100644
--- a/drivers/platform/x86/asus-laptop.c
+++ b/drivers/platform/x86/asus-laptop.c
@@ -932,30 +932,19 @@ static ssize_t infos_show(struct device *dev, struct 
device_attribute *attr,
 }
 static DEVICE_ATTR_RO(infos);
 
-static int parse_arg(const char *buf, unsigned long count, int *val)
-{
-   if (!count)
-   return 0;
-   if (count > 31)
-   return -EINVAL;
-   if (sscanf(buf, "%i", val) != 1)
-   return -EINVAL;
-   return count;
-}
-
 static ssize_t sysfs_acpi_set(struct asus_laptop *asus,
  const char *buf, size_t count,
  const char *method)
 {
int rv, value;
 
-   rv = parse_arg(buf, count, );
-   if (rv <= 0)
+   rv = kstrtoint(buf, 0, );
+   if (rv < 0)
return rv;
 
if (write_acpi_int(asus->handle, method, value))
return -ENODEV;
-   return rv;
+   return count;
 }
 
 /*
@@ -975,15 +964,17 @@ static ssize_t ledd_store(struct device *dev, struct 
device_attribute *attr,
struct asus_laptop *asus = dev_get_drvdata(dev);
int rv, value;
 
-   rv = parse_arg(buf, count, );
-   if (rv > 0) {
-   if (write_acpi_int(asus->handle, METHOD_LEDD, value)) {
-   pr_warn("LED display write failed\n");
-   return -ENODEV;
-   }
-   asus->ledd_status = (u32) value;
+   rv = kstrtoint(buf, 0, );
+   if (rv < 0)
+   return rv;
+
+   if (write_acpi_int(asus->handle, METHOD_LEDD, value)) {
+   pr_warn("LED display write failed\n");
+   return -ENODEV;
}
-   return rv;
+
+   asus->ledd_status = (u32) value;
+   return count;
 }
 static DEVICE_ATTR_RW(ledd);
 
@@ -1148,10 +1139,12 @@ static ssize_t display_store(struct device *dev, struct 
device_attribute *attr,
struct asus_laptop *asus = dev_get_drvdata(dev);
int rv, value;
 
-   rv = parse_arg(buf, count, );
-   if (rv > 0)
-   asus_set_display(asus, value);
-   return rv;
+   rv = kstrtoint(buf, 0, );
+   if (rv < 0)
+   return rv;
+
+   asus_set_display(asus, value);
+   return count;
 }
 static DEVICE_ATTR_WO(display);
 
@@ -1190,11 +1183,12 @@ static ssize_t ls_switch_store(struct device *dev,
struct asus_laptop *asus = dev_get_drvdata(dev);
int rv, value;
 
-   rv = parse_arg(buf, count, );
-   if (rv > 0)
-   asus_als_switch(asus, value ? 1 : 0);
+   rv = kstrtoint(buf, 0, );
+   if (rv < 0)
+   return rv;
 
-   return rv;
+   asus_als_switch(asus, value ? 1 : 0);
+   return count;
 }
 static DEVICE_ATTR_RW(ls_switch);
 
@@ -1219,14 +1213,15 @@ static ssize_t ls_level_store(struct device *dev, 
struct device_attribute *attr,
struct asus_laptop *asus = dev_get_drvdata(dev);
int rv, value;
 
-   rv = parse_arg(buf, count, );
-   if (rv > 0) {
-   value = (0 < value) ? ((15 < value) ? 15 : value) : 0;
-   /* 0 <= value <= 15 */
-   asus_als_level(asus, value);
-   }
+   rv = kstrtoint(buf, 0, );
+   if (rv < 0)
+   

[PATCH] asus-laptop: get rid of parse_arg()

[Giedrius Statkevičius]

parse_arg() duplicates the funcionality of kstrtoint() so use the latter
function instead. There is no funcionality change except that in the
case of input being too big -ERANGE will be returned instead of -EINVAL
which is not bad because -ERANGE makes more sense here. The check for
!count is already done by the sysfs core so no need to duplicate it
again. Also, add some minor corrections to error handling to accommodate
the change in return values (parse_arg returned count if everything
succeeded whereas kstrtoint returns 0 in the same situation)

As a result of this patch asus-laptop.ko size is reduced by almost 1%:
add/remove: 0/1 grow/shrink: 1/6 up/down: 1/-149 (-148)
function old new   delta
__UNIQUE_ID_vermagic0 69  70  +1
ls_switch_store  133 117 -16
ledd_store   175 159 -16
display_store157 141 -16
ls_level_store   193 176 -17
gps_store200 178 -22
sysfs_acpi_set.isra  148 125 -23
parse_arg.part39   - -39
Total: Before=19160, After=19012, chg -0.77%

Signed-off-by: Giedrius Statkevičius 
---
 drivers/platform/x86/asus-laptop.c | 77 ++
 1 file changed, 36 insertions(+), 41 deletions(-)

diff --git a/drivers/platform/x86/asus-laptop.c 
b/drivers/platform/x86/asus-laptop.c
index 15f1311..28551f5 100644
--- a/drivers/platform/x86/asus-laptop.c
+++ b/drivers/platform/x86/asus-laptop.c
@@ -932,30 +932,19 @@ static ssize_t infos_show(struct device *dev, struct 
device_attribute *attr,
 }
 static DEVICE_ATTR_RO(infos);
 
-static int parse_arg(const char *buf, unsigned long count, int *val)
-{
-   if (!count)
-   return 0;
-   if (count > 31)
-   return -EINVAL;
-   if (sscanf(buf, "%i", val) != 1)
-   return -EINVAL;
-   return count;
-}
-
 static ssize_t sysfs_acpi_set(struct asus_laptop *asus,
  const char *buf, size_t count,
  const char *method)
 {
int rv, value;
 
-   rv = parse_arg(buf, count, );
-   if (rv <= 0)
+   rv = kstrtoint(buf, 0, );
+   if (rv < 0)
return rv;
 
if (write_acpi_int(asus->handle, method, value))
return -ENODEV;
-   return rv;
+   return count;
 }
 
 /*
@@ -975,15 +964,17 @@ static ssize_t ledd_store(struct device *dev, struct 
device_attribute *attr,
struct asus_laptop *asus = dev_get_drvdata(dev);
int rv, value;
 
-   rv = parse_arg(buf, count, );
-   if (rv > 0) {
-   if (write_acpi_int(asus->handle, METHOD_LEDD, value)) {
-   pr_warn("LED display write failed\n");
-   return -ENODEV;
-   }
-   asus->ledd_status = (u32) value;
+   rv = kstrtoint(buf, 0, );
+   if (rv < 0)
+   return rv;
+
+   if (write_acpi_int(asus->handle, METHOD_LEDD, value)) {
+   pr_warn("LED display write failed\n");
+   return -ENODEV;
}
-   return rv;
+
+   asus->ledd_status = (u32) value;
+   return count;
 }
 static DEVICE_ATTR_RW(ledd);
 
@@ -1148,10 +1139,12 @@ static ssize_t display_store(struct device *dev, struct 
device_attribute *attr,
struct asus_laptop *asus = dev_get_drvdata(dev);
int rv, value;
 
-   rv = parse_arg(buf, count, );
-   if (rv > 0)
-   asus_set_display(asus, value);
-   return rv;
+   rv = kstrtoint(buf, 0, );
+   if (rv < 0)
+   return rv;
+
+   asus_set_display(asus, value);
+   return count;
 }
 static DEVICE_ATTR_WO(display);
 
@@ -1190,11 +1183,12 @@ static ssize_t ls_switch_store(struct device *dev,
struct asus_laptop *asus = dev_get_drvdata(dev);
int rv, value;
 
-   rv = parse_arg(buf, count, );
-   if (rv > 0)
-   asus_als_switch(asus, value ? 1 : 0);
+   rv = kstrtoint(buf, 0, );
+   if (rv < 0)
+   return rv;
 
-   return rv;
+   asus_als_switch(asus, value ? 1 : 0);
+   return count;
 }
 static DEVICE_ATTR_RW(ls_switch);
 
@@ -1219,14 +1213,15 @@ static ssize_t ls_level_store(struct device *dev, 
struct device_attribute *attr,
struct asus_laptop *asus = dev_get_drvdata(dev);
int rv, value;
 
-   rv = parse_arg(buf, count, );
-   if (rv > 0) {
-   value = (0 < value) ? ((15 < value) ? 15 : value) : 0;
-   /* 0 <= value <= 15 */
-   asus_als_level(asus, value);
-   }
+   rv = kstrtoint(buf, 0, );
+   if (rv < 0)
+   return rv;
+

Re: [PATCH 1651/1651] staging: i4l: pcbit: Remove explicit NULL comparison

[Giedrius Statkevičius]

Hello shyam saini,

On Tue, Aug 2, 2016 at 2:33 PM, shyam saini  wrote:
> Remove the explicit NULL comparison and rewrite in a compact form using 
> Coccinelle
>
> Signed-off-by: shyam saini 

Where are the other 1650 patches? Also, a patch series that consists
of 1651 patches???

Re: [PATCH 1651/1651] staging: i4l: pcbit: Remove explicit NULL comparison

[Giedrius Statkevičius]

Hello shyam saini,

On Tue, Aug 2, 2016 at 2:33 PM, shyam saini  wrote:
> Remove the explicit NULL comparison and rewrite in a compact form using 
> Coccinelle
>
> Signed-off-by: shyam saini 

Where are the other 1650 patches? Also, a patch series that consists
of 1651 patches???

Re: [PATCH] Staging: android: ion: ion.c: Compression of lines for

[Giedrius Statkevičius]

On Sun, Jul 31, 2016 at 6:44 PM, Nadim almas  wrote:
> This patch compresses two lines in to a single line in file
> ion.c
> if immediate return statement is found.It also removes variable
> ret as it is no longer needed.
>
> ne using script Coccinelle. And coccinelle uses following semantic
> patch for this compression function:
>
> @@
> expression e, ret;
> @@
>
> -ret =
> +return
>  e;
> -return ret;
>
> Signed-off-by: Nadim Almas 
> ---
>  drivers/staging/android/ion/ion.c  | 8 +++-
>
>  1 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/android/ion/ion.c 
> b/drivers/staging/android/ion/ion.c
> index 52345df..271395b 100644
> --- a/drivers/staging/android/ion/ion.c
> +++ b/drivers/staging/android/ion/ion.c
> @@ -391,9 +391,7 @@ static int ion_handle_put_nolock(struct ion_handle 
> *handle)
>  {
> -   int ret;
>
> -   ret = kref_put(>ref, ion_handle_destroy);
> -
> -   return ret;
> +   return kref_put(>ref, ion_handle_destroy);
>  }
>
>  static int ion_handle_put(struct ion_handle *handle)
> @@ -597,8 +595,8 @@ int ion_phys(struct ion_client *client, struct ion_handle 
> *handle,
> return -ENODEV;
> }
> mutex_unlock(>lock);
> -   ret = buffer->heap->ops->phys(buffer->heap, buffer, addr, len);
> -   return ret;
> +   return buffer->heap->ops->phys(buffer->heap, buffer, addr, len);
>  }

Try to actually compile before sending your patch.

Re: [PATCH] Staging: android: ion: ion.c: Compression of lines for

[Giedrius Statkevičius]

On Sun, Jul 31, 2016 at 6:44 PM, Nadim almas  wrote:
> This patch compresses two lines in to a single line in file
> ion.c
> if immediate return statement is found.It also removes variable
> ret as it is no longer needed.
>
> ne using script Coccinelle. And coccinelle uses following semantic
> patch for this compression function:
>
> @@
> expression e, ret;
> @@
>
> -ret =
> +return
>  e;
> -return ret;
>
> Signed-off-by: Nadim Almas 
> ---
>  drivers/staging/android/ion/ion.c  | 8 +++-
>
>  1 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/android/ion/ion.c 
> b/drivers/staging/android/ion/ion.c
> index 52345df..271395b 100644
> --- a/drivers/staging/android/ion/ion.c
> +++ b/drivers/staging/android/ion/ion.c
> @@ -391,9 +391,7 @@ static int ion_handle_put_nolock(struct ion_handle 
> *handle)
>  {
> -   int ret;
>
> -   ret = kref_put(>ref, ion_handle_destroy);
> -
> -   return ret;
> +   return kref_put(>ref, ion_handle_destroy);
>  }
>
>  static int ion_handle_put(struct ion_handle *handle)
> @@ -597,8 +595,8 @@ int ion_phys(struct ion_client *client, struct ion_handle 
> *handle,
> return -ENODEV;
> }
> mutex_unlock(>lock);
> -   ret = buffer->heap->ops->phys(buffer->heap, buffer, addr, len);
> -   return ret;
> +   return buffer->heap->ops->phys(buffer->heap, buffer, addr, len);
>  }

Try to actually compile before sending your patch.

Re: [PATCH v2] rtl8712: Fixed alignment to match open parenthesis

[Giedrius Statkevičius]

On Mon, Apr 25, 2016 at 4:00 AM, Parth Sane  wrote:
> Added missing signed off by line and fixed alignment to match
> open parenthesis.

Put "Added missing signed off by line" below the ---. This is not what
we want to have in change logs.

Re: [PATCH v2] rtl8712: Fixed alignment to match open parenthesis

[Giedrius Statkevičius]

On Mon, Apr 25, 2016 at 4:00 AM, Parth Sane  wrote:
> Added missing signed off by line and fixed alignment to match
> open parenthesis.

Put "Added missing signed off by line" below the ---. This is not what
we want to have in change logs.

Re: [PATCH] asus-laptop: correct error handling in asus_read_brightness()

[Giedrius Statkevičius]

On Fri, Apr 22, 2016 at 02:09:22AM +0300, Andy Shevchenko wrote:
> On Sat, Apr 16, 2016 at 3:27 AM, Giedrius Statkevičius
> <giedrius.statkevic...@gmail.com> wrote:
> > It is possible that acpi_evaluate_integer might fail and value would not be
> > set to any value so correct this defect by returning 0 in case of an
> > error. This is also the correct thing to return because the backlight
> > subsystem will print the old value of brightness in this case.
> >
> > Signed-off-by: Giedrius Statkevičius <giedrius.statkevic...@gmail.com>
> > ---
> >  drivers/platform/x86/asus-laptop.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/platform/x86/asus-laptop.c 
> > b/drivers/platform/x86/asus-laptop.c
> > index 9a69734..15f1311 100644
> > --- a/drivers/platform/x86/asus-laptop.c
> > +++ b/drivers/platform/x86/asus-laptop.c
> > @@ -775,8 +775,10 @@ static int asus_read_brightness(struct 
> > backlight_device *bd)
> >
> > rv = acpi_evaluate_integer(asus->handle, METHOD_BRIGHTNESS_GET,
> >NULL, );
> > -   if (ACPI_FAILURE(rv))
> > +   if (ACPI_FAILURE(rv)) {
> > pr_warn("Error reading brightness\n");
> > +   return 0;
> > +   }
> 
> This looks like a workaround.
> I suppose the real fix is to return here an error code and fix all callers, 
> like
> drivers/video/backlight/backlight.c.
> 

It just fixes the behaviour according to the current code in that file. I
suppose that would be nice but I don't think it would make any difference
because the backlight core code still prints out ->props.brightness in case
ops->get_brightness fails. Just the difference would be that now actual error
messages are printed in the drivers themselves instead of generic messages from
the backlight core. Anyway, I think the current behaviour is more useful because
the drivers know better about what has failed.

Re: [PATCH] asus-laptop: correct error handling in asus_read_brightness()

[Giedrius Statkevičius]

On Fri, Apr 22, 2016 at 02:09:22AM +0300, Andy Shevchenko wrote:
> On Sat, Apr 16, 2016 at 3:27 AM, Giedrius Statkevičius
>  wrote:
> > It is possible that acpi_evaluate_integer might fail and value would not be
> > set to any value so correct this defect by returning 0 in case of an
> > error. This is also the correct thing to return because the backlight
> > subsystem will print the old value of brightness in this case.
> >
> > Signed-off-by: Giedrius Statkevičius 
> > ---
> >  drivers/platform/x86/asus-laptop.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/platform/x86/asus-laptop.c 
> > b/drivers/platform/x86/asus-laptop.c
> > index 9a69734..15f1311 100644
> > --- a/drivers/platform/x86/asus-laptop.c
> > +++ b/drivers/platform/x86/asus-laptop.c
> > @@ -775,8 +775,10 @@ static int asus_read_brightness(struct 
> > backlight_device *bd)
> >
> > rv = acpi_evaluate_integer(asus->handle, METHOD_BRIGHTNESS_GET,
> >NULL, );
> > -   if (ACPI_FAILURE(rv))
> > +   if (ACPI_FAILURE(rv)) {
> > pr_warn("Error reading brightness\n");
> > +   return 0;
> > +   }
> 
> This looks like a workaround.
> I suppose the real fix is to return here an error code and fix all callers, 
> like
> drivers/video/backlight/backlight.c.
> 

It just fixes the behaviour according to the current code in that file. I
suppose that would be nice but I don't think it would make any difference
because the backlight core code still prints out ->props.brightness in case
ops->get_brightness fails. Just the difference would be that now actual error
messages are printed in the drivers themselves instead of generic messages from
the backlight core. Anyway, I think the current behaviour is more useful because
the drivers know better about what has failed.

Re: [PATCH v2 2/2] asus-laptop: correct error handling in sysfs_acpi_set

[Giedrius Statkevičius]

On Wed, Apr 20, 2016 at 01:19:55PM -0700, Darren Hart wrote:
> On Sat, Apr 16, 2016 at 03:01:57AM +0300, Giedrius Statkevičius wrote:
> > Properly return rv back to the caller in the case of an error in
> > parse_arg. In the process remove a unused variable 'out'.
> 
> The initial problem if I recall was value being uninitialized. Is that 
> correct?
No, 'out' was just removed as it was unused. Then you caught the issue with
error handling in this function so I've updated this patch to fix that as well.

Re: [PATCH v2 2/2] asus-laptop: correct error handling in sysfs_acpi_set

[Giedrius Statkevičius]

On Wed, Apr 20, 2016 at 01:19:55PM -0700, Darren Hart wrote:
> On Sat, Apr 16, 2016 at 03:01:57AM +0300, Giedrius Statkevičius wrote:
> > Properly return rv back to the caller in the case of an error in
> > parse_arg. In the process remove a unused variable 'out'.
> 
> The initial problem if I recall was value being uninitialized. Is that 
> correct?
No, 'out' was just removed as it was unused. Then you caught the issue with
error handling in this function so I've updated this patch to fix that as well.

[PATCH] asus-laptop: correct error handling in asus_read_brightness()

[Giedrius Statkevičius]

It is possible that acpi_evaluate_integer might fail and value would not be
set to any value so correct this defect by returning 0 in case of an
error. This is also the correct thing to return because the backlight
subsystem will print the old value of brightness in this case.

Signed-off-by: Giedrius Statkevičius <giedrius.statkevic...@gmail.com>
---
 drivers/platform/x86/asus-laptop.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/asus-laptop.c 
b/drivers/platform/x86/asus-laptop.c
index 9a69734..15f1311 100644
--- a/drivers/platform/x86/asus-laptop.c
+++ b/drivers/platform/x86/asus-laptop.c
@@ -775,8 +775,10 @@ static int asus_read_brightness(struct backlight_device 
*bd)
 
rv = acpi_evaluate_integer(asus->handle, METHOD_BRIGHTNESS_GET,
   NULL, );
-   if (ACPI_FAILURE(rv))
+   if (ACPI_FAILURE(rv)) {
pr_warn("Error reading brightness\n");
+   return 0;
+   }
 
return value;
 }
-- 
2.8.0

[PATCH] asus-laptop: correct error handling in asus_read_brightness()

[Giedrius Statkevičius]

It is possible that acpi_evaluate_integer might fail and value would not be
set to any value so correct this defect by returning 0 in case of an
error. This is also the correct thing to return because the backlight
subsystem will print the old value of brightness in this case.

Signed-off-by: Giedrius Statkevičius 
---
 drivers/platform/x86/asus-laptop.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/asus-laptop.c 
b/drivers/platform/x86/asus-laptop.c
index 9a69734..15f1311 100644
--- a/drivers/platform/x86/asus-laptop.c
+++ b/drivers/platform/x86/asus-laptop.c
@@ -775,8 +775,10 @@ static int asus_read_brightness(struct backlight_device 
*bd)
 
rv = acpi_evaluate_integer(asus->handle, METHOD_BRIGHTNESS_GET,
   NULL, );
-   if (ACPI_FAILURE(rv))
+   if (ACPI_FAILURE(rv)) {
pr_warn("Error reading brightness\n");
+   return 0;
+   }
 
return value;
 }
-- 
2.8.0

[PATCH v2 2/2] asus-laptop: correct error handling in sysfs_acpi_set

[Giedrius Statkevičius]

Properly return rv back to the caller in the case of an error in
parse_arg. In the process remove a unused variable 'out'.

Signed-off-by: Giedrius Statkevičius <giedrius.statkevic...@gmail.com>
---
 drivers/platform/x86/asus-laptop.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/asus-laptop.c 
b/drivers/platform/x86/asus-laptop.c
index d86d42e..9a69734 100644
--- a/drivers/platform/x86/asus-laptop.c
+++ b/drivers/platform/x86/asus-laptop.c
@@ -946,11 +946,10 @@ static ssize_t sysfs_acpi_set(struct asus_laptop *asus,
  const char *method)
 {
int rv, value;
-   int out = 0;
 
rv = parse_arg(buf, count, );
-   if (rv > 0)
-   out = value ? 1 : 0;
+   if (rv <= 0)
+   return rv;
 
if (write_acpi_int(asus->handle, method, value))
return -ENODEV;
-- 
2.8.0

[PATCH v2 2/2] asus-laptop: correct error handling in sysfs_acpi_set

[Giedrius Statkevičius]

Properly return rv back to the caller in the case of an error in
parse_arg. In the process remove a unused variable 'out'.

Signed-off-by: Giedrius Statkevičius 
---
 drivers/platform/x86/asus-laptop.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/asus-laptop.c 
b/drivers/platform/x86/asus-laptop.c
index d86d42e..9a69734 100644
--- a/drivers/platform/x86/asus-laptop.c
+++ b/drivers/platform/x86/asus-laptop.c
@@ -946,11 +946,10 @@ static ssize_t sysfs_acpi_set(struct asus_laptop *asus,
  const char *method)
 {
int rv, value;
-   int out = 0;
 
rv = parse_arg(buf, count, );
-   if (rv > 0)
-   out = value ? 1 : 0;
+   if (rv <= 0)
+   return rv;
 
if (write_acpi_int(asus->handle, method, value))
return -ENODEV;
-- 
2.8.0

[PATCH v2 1/2] asus-laptop: remove redundant initializers

[Giedrius Statkevičius]

Initializing rv to AE_OK is pointless because later function results are
assigned to them and only then the variable is used

Signed-off-by: Giedrius Statkevičius <giedrius.statkevic...@gmail.com>
---
 drivers/platform/x86/asus-laptop.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/asus-laptop.c 
b/drivers/platform/x86/asus-laptop.c
index f2b5d0a..d86d42e 100644
--- a/drivers/platform/x86/asus-laptop.c
+++ b/drivers/platform/x86/asus-laptop.c
@@ -771,7 +771,7 @@ static int asus_read_brightness(struct backlight_device *bd)
 {
struct asus_laptop *asus = bl_get_data(bd);
unsigned long long value;
-   acpi_status rv = AE_OK;
+   acpi_status rv;
 
rv = acpi_evaluate_integer(asus->handle, METHOD_BRIGHTNESS_GET,
   NULL, );
@@ -865,7 +865,7 @@ static ssize_t infos_show(struct device *dev, struct 
device_attribute *attr,
int len = 0;
unsigned long long temp;
char buf[16];   /* enough for all info */
-   acpi_status rv = AE_OK;
+   acpi_status rv;
 
/*
 * We use the easy way, we don't care of off and count,
@@ -1265,7 +1265,7 @@ static DEVICE_ATTR_RO(ls_value);
 static int asus_gps_status(struct asus_laptop *asus)
 {
unsigned long long status;
-   acpi_status rv = AE_OK;
+   acpi_status rv;
 
rv = acpi_evaluate_integer(asus->handle, METHOD_GPS_STATUS,
   NULL, );
-- 
2.8.0

[PATCH v2 1/2] asus-laptop: remove redundant initializers

[Giedrius Statkevičius]

Initializing rv to AE_OK is pointless because later function results are
assigned to them and only then the variable is used

Signed-off-by: Giedrius Statkevičius 
---
 drivers/platform/x86/asus-laptop.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/asus-laptop.c 
b/drivers/platform/x86/asus-laptop.c
index f2b5d0a..d86d42e 100644
--- a/drivers/platform/x86/asus-laptop.c
+++ b/drivers/platform/x86/asus-laptop.c
@@ -771,7 +771,7 @@ static int asus_read_brightness(struct backlight_device *bd)
 {
struct asus_laptop *asus = bl_get_data(bd);
unsigned long long value;
-   acpi_status rv = AE_OK;
+   acpi_status rv;
 
rv = acpi_evaluate_integer(asus->handle, METHOD_BRIGHTNESS_GET,
   NULL, );
@@ -865,7 +865,7 @@ static ssize_t infos_show(struct device *dev, struct 
device_attribute *attr,
int len = 0;
unsigned long long temp;
char buf[16];   /* enough for all info */
-   acpi_status rv = AE_OK;
+   acpi_status rv;
 
/*
 * We use the easy way, we don't care of off and count,
@@ -1265,7 +1265,7 @@ static DEVICE_ATTR_RO(ls_value);
 static int asus_gps_status(struct asus_laptop *asus)
 {
unsigned long long status;
-   acpi_status rv = AE_OK;
+   acpi_status rv;
 
rv = acpi_evaluate_integer(asus->handle, METHOD_GPS_STATUS,
   NULL, );
-- 
2.8.0

Re: [PATCH 2/2] asus-laptop: remove unused variable

[Giedrius Statkevičius]

On Sat, Apr 09, 2016 at 08:21:21PM -0700, Darren Hart wrote:
> On Thu, Apr 07, 2016 at 11:20:01PM +0300, Giedrius Statkevičius wrote:
> > `out' was assigned value but it was never used so remove it
> > 
> > Signed-off-by: Giedrius Statkevičius <giedrius.statkevic...@gmail.com>
> > ---
> >  drivers/platform/x86/asus-laptop.c | 3 ---
> >  1 file changed, 3 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/asus-laptop.c 
> > b/drivers/platform/x86/asus-laptop.c
> > index d86d42e..39ddcee 100644
> > --- a/drivers/platform/x86/asus-laptop.c
> > +++ b/drivers/platform/x86/asus-laptop.c
> > @@ -946,11 +946,8 @@ static ssize_t sysfs_acpi_set(struct asus_laptop *asus,
> >   const char *method)
> >  {
> > int rv, value;
> > -   int out = 0;
> >  
> > rv = parse_arg(buf, count, );
> > -   if (rv > 0)
> > -   out = value ? 1 : 0;
> >  
> > if (write_acpi_int(asus->handle, method, value))
> 
> out is indeed unused, however the rv > 0 condition is relevant as <=0 will 
> pass
> value uninitialized to write_acpi_int.
That's indeed a problem. Somehow I missed that :( Should I make a v2 to include
another patch that checks for rv < 0 or send it as an independent one?

Re: [PATCH 2/2] asus-laptop: remove unused variable

[Giedrius Statkevičius]

On Sat, Apr 09, 2016 at 08:21:21PM -0700, Darren Hart wrote:
> On Thu, Apr 07, 2016 at 11:20:01PM +0300, Giedrius Statkevičius wrote:
> > `out' was assigned value but it was never used so remove it
> > 
> > Signed-off-by: Giedrius Statkevičius 
> > ---
> >  drivers/platform/x86/asus-laptop.c | 3 ---
> >  1 file changed, 3 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/asus-laptop.c 
> > b/drivers/platform/x86/asus-laptop.c
> > index d86d42e..39ddcee 100644
> > --- a/drivers/platform/x86/asus-laptop.c
> > +++ b/drivers/platform/x86/asus-laptop.c
> > @@ -946,11 +946,8 @@ static ssize_t sysfs_acpi_set(struct asus_laptop *asus,
> >   const char *method)
> >  {
> > int rv, value;
> > -   int out = 0;
> >  
> > rv = parse_arg(buf, count, );
> > -   if (rv > 0)
> > -   out = value ? 1 : 0;
> >  
> > if (write_acpi_int(asus->handle, method, value))
> 
> out is indeed unused, however the rv > 0 condition is relevant as <=0 will 
> pass
> value uninitialized to write_acpi_int.
That's indeed a problem. Somehow I missed that :( Should I make a v2 to include
another patch that checks for rv < 0 or send it as an independent one?

[PATCH 1/2] asus-laptop: remove redundant initializers

[Giedrius Statkevičius]

Initializing rv to AE_OK is pointless because later function results are
assigned to them and only then the variable is used

Signed-off-by: Giedrius Statkevičius <giedrius.statkevic...@gmail.com>
---
 drivers/platform/x86/asus-laptop.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/asus-laptop.c 
b/drivers/platform/x86/asus-laptop.c
index f2b5d0a..d86d42e 100644
--- a/drivers/platform/x86/asus-laptop.c
+++ b/drivers/platform/x86/asus-laptop.c
@@ -771,7 +771,7 @@ static int asus_read_brightness(struct backlight_device *bd)
 {
struct asus_laptop *asus = bl_get_data(bd);
unsigned long long value;
-   acpi_status rv = AE_OK;
+   acpi_status rv;
 
rv = acpi_evaluate_integer(asus->handle, METHOD_BRIGHTNESS_GET,
   NULL, );
@@ -865,7 +865,7 @@ static ssize_t infos_show(struct device *dev, struct 
device_attribute *attr,
int len = 0;
unsigned long long temp;
char buf[16];   /* enough for all info */
-   acpi_status rv = AE_OK;
+   acpi_status rv;
 
/*
 * We use the easy way, we don't care of off and count,
@@ -1265,7 +1265,7 @@ static DEVICE_ATTR_RO(ls_value);
 static int asus_gps_status(struct asus_laptop *asus)
 {
unsigned long long status;
-   acpi_status rv = AE_OK;
+   acpi_status rv;
 
rv = acpi_evaluate_integer(asus->handle, METHOD_GPS_STATUS,
   NULL, );
-- 
2.8.0

[PATCH 2/2] asus-laptop: remove unused variable

[Giedrius Statkevičius]

`out' was assigned value but it was never used so remove it

Signed-off-by: Giedrius Statkevičius <giedrius.statkevic...@gmail.com>
---
 drivers/platform/x86/asus-laptop.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/platform/x86/asus-laptop.c 
b/drivers/platform/x86/asus-laptop.c
index d86d42e..39ddcee 100644
--- a/drivers/platform/x86/asus-laptop.c
+++ b/drivers/platform/x86/asus-laptop.c
@@ -946,11 +946,8 @@ static ssize_t sysfs_acpi_set(struct asus_laptop *asus,
  const char *method)
 {
int rv, value;
-   int out = 0;
 
rv = parse_arg(buf, count, );
-   if (rv > 0)
-   out = value ? 1 : 0;
 
if (write_acpi_int(asus->handle, method, value))
return -ENODEV;
-- 
2.8.0

[PATCH 2/2] asus-laptop: remove unused variable

[Giedrius Statkevičius]

`out' was assigned value but it was never used so remove it

Signed-off-by: Giedrius Statkevičius 
---
 drivers/platform/x86/asus-laptop.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/platform/x86/asus-laptop.c 
b/drivers/platform/x86/asus-laptop.c
index d86d42e..39ddcee 100644
--- a/drivers/platform/x86/asus-laptop.c
+++ b/drivers/platform/x86/asus-laptop.c
@@ -946,11 +946,8 @@ static ssize_t sysfs_acpi_set(struct asus_laptop *asus,
  const char *method)
 {
int rv, value;
-   int out = 0;
 
rv = parse_arg(buf, count, );
-   if (rv > 0)
-   out = value ? 1 : 0;
 
if (write_acpi_int(asus->handle, method, value))
return -ENODEV;
-- 
2.8.0

[PATCH 1/2] asus-laptop: remove redundant initializers

[Giedrius Statkevičius]

Initializing rv to AE_OK is pointless because later function results are
assigned to them and only then the variable is used

Signed-off-by: Giedrius Statkevičius 
---
 drivers/platform/x86/asus-laptop.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/asus-laptop.c 
b/drivers/platform/x86/asus-laptop.c
index f2b5d0a..d86d42e 100644
--- a/drivers/platform/x86/asus-laptop.c
+++ b/drivers/platform/x86/asus-laptop.c
@@ -771,7 +771,7 @@ static int asus_read_brightness(struct backlight_device *bd)
 {
struct asus_laptop *asus = bl_get_data(bd);
unsigned long long value;
-   acpi_status rv = AE_OK;
+   acpi_status rv;
 
rv = acpi_evaluate_integer(asus->handle, METHOD_BRIGHTNESS_GET,
   NULL, );
@@ -865,7 +865,7 @@ static ssize_t infos_show(struct device *dev, struct 
device_attribute *attr,
int len = 0;
unsigned long long temp;
char buf[16];   /* enough for all info */
-   acpi_status rv = AE_OK;
+   acpi_status rv;
 
/*
 * We use the easy way, we don't care of off and count,
@@ -1265,7 +1265,7 @@ static DEVICE_ATTR_RO(ls_value);
 static int asus_gps_status(struct asus_laptop *asus)
 {
unsigned long long status;
-   acpi_status rv = AE_OK;
+   acpi_status rv;
 
rv = acpi_evaluate_integer(asus->handle, METHOD_GPS_STATUS,
   NULL, );
-- 
2.8.0

Re: [PATCH][RESEND] asus-nb-wmi: add wapf=4 quirk for ASUS X75VD

[Giedrius Statkevičius]

On Wed, Feb 17, 2016 at 09:45:05AM +0200, Oleksandr Natalenko wrote:
> Wi-Fi on ASUS X75VD laptop does not work unless asus_nb_wmi module
> is loaded with wapf=4 option. Add quirk for this.
> ---

You forgot Signed-off-by here. Always run your patch through
scripts/checkpatch.pl. Also, I think the format is usually
[PATCH RESEND vX AA/BB] and not [PATCH][RESEND].

--
Giedrius

Re: [PATCH][RESEND] asus-nb-wmi: add wapf=4 quirk for ASUS X75VD

[Giedrius Statkevičius]

On Wed, Feb 17, 2016 at 09:45:05AM +0200, Oleksandr Natalenko wrote:
> Wi-Fi on ASUS X75VD laptop does not work unless asus_nb_wmi module
> is loaded with wapf=4 option. Add quirk for this.
> ---

You forgot Signed-off-by here. Always run your patch through
scripts/checkpatch.pl. Also, I think the format is usually
[PATCH RESEND vX AA/BB] and not [PATCH][RESEND].

--
Giedrius

Re: 30c2a1faaeb3db94fc92f79553cc72634aa3b218 broke cryptsetup on my machine

[Giedrius Statkevičius]

On Thu, 8 Oct 2015, Geliang Tang wrote:

> On Wed, Oct 07, 2015 at 10:27:27PM +0300, Giedrius Statkevičius wrote:
> > Hello,
> > I have a LUKS on LVM setup: /boot is unencrypted and everything is "hidden" 
> > in
> > /dev/sda2. After booting on linux-next and just after entering my password
> > cryptsetup segfaults and a stack trace is printed from the kernel. Since 
> > writing
> > down all those numbers is hard I've made a picture where it is shown:
> > https://i.imgur.com/6PHNUdv.jpg
> > 
> > I figured it had something to do with changes to memory management and thus 
> > find
> > out that commit 30c2a1faaeb3db94fc92f79553cc72634aa3b218 broke my system.
> > Reverting it there is no such error anymore and system boots fine past that
> > point. 
> > 
> > Could someone look into this? Thanks, Giedrius
> 
> Bugfix for this has been sended out.
> https://lkml.org/lkml/2015/10/8/342.
> 
> Thanks.
I've tested and that fixed this issue for me. I would've replied on that thread
but I can't find its Message-Id. Anyway, you can add:
Tested-By: Giedrius Statkevičius 

Thanks,
Giedrius

Re: 30c2a1faaeb3db94fc92f79553cc72634aa3b218 broke cryptsetup on my machine

[Giedrius Statkevičius]

On Thu, 8 Oct 2015, Geliang Tang wrote:

> On Wed, Oct 07, 2015 at 10:27:27PM +0300, Giedrius Statkevičius wrote:
> > Hello,
> > I have a LUKS on LVM setup: /boot is unencrypted and everything is "hidden" 
> > in
> > /dev/sda2. After booting on linux-next and just after entering my password
> > cryptsetup segfaults and a stack trace is printed from the kernel. Since 
> > writing
> > down all those numbers is hard I've made a picture where it is shown:
> > https://i.imgur.com/6PHNUdv.jpg
> > 
> > I figured it had something to do with changes to memory management and thus 
> > find
> > out that commit 30c2a1faaeb3db94fc92f79553cc72634aa3b218 broke my system.
> > Reverting it there is no such error anymore and system boots fine past that
> > point. 
> > 
> > Could someone look into this? Thanks, Giedrius
> 
> Bugfix for this has been sended out.
> https://lkml.org/lkml/2015/10/8/342.
> 
> Thanks.
I've tested and that fixed this issue for me. I would've replied on that thread
but I can't find its Message-Id. Anyway, you can add:
Tested-By: Giedrius Statkevičius <giedrius.statkevic...@gmail.com>

Thanks,
Giedrius

30c2a1faaeb3db94fc92f79553cc72634aa3b218 broke cryptsetup on my machine

[Giedrius Statkevičius]

Hello,
I have a LUKS on LVM setup: /boot is unencrypted and everything is "hidden" in
/dev/sda2. After booting on linux-next and just after entering my password
cryptsetup segfaults and a stack trace is printed from the kernel. Since writing
down all those numbers is hard I've made a picture where it is shown:
https://i.imgur.com/6PHNUdv.jpg

I figured it had something to do with changes to memory management and thus find
out that commit 30c2a1faaeb3db94fc92f79553cc72634aa3b218 broke my system.
Reverting it there is no such error anymore and system boots fine past that
point. 

Could someone look into this? Thanks, Giedrius
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

30c2a1faaeb3db94fc92f79553cc72634aa3b218 broke cryptsetup on my machine

[Giedrius Statkevičius]

Hello,
I have a LUKS on LVM setup: /boot is unencrypted and everything is "hidden" in
/dev/sda2. After booting on linux-next and just after entering my password
cryptsetup segfaults and a stack trace is printed from the kernel. Since writing
down all those numbers is hard I've made a picture where it is shown:
https://i.imgur.com/6PHNUdv.jpg

I figured it had something to do with changes to memory management and thus find
out that commit 30c2a1faaeb3db94fc92f79553cc72634aa3b218 broke my system.
Reverting it there is no such error anymore and system boots fine past that
point. 

Could someone look into this? Thanks, Giedrius
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/8] fix staging:android style issue:Alignment should match open parenthesis

[Giedrius Statkevičius]

On Wed, Aug 26, 2015 at 11:52:12AM +0800, Peng Sun wrote:
> Signed-off-by: Peng Sun 

Please always add a changelog message. Also, you need to fix the subject. The
convention is to use something like: "a: b: ..."
So in this case it should be something like: "staging: android: ..."
You can look at `git log drivers/staging/android` for inspiration.  This applies
to all of your other patches as well.  Finally, I don't think there is a point
in using deep threading in this case.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/8] fix staging:android style issue:Alignment should match open parenthesis

[Giedrius Statkevičius]

On Wed, Aug 26, 2015 at 11:52:12AM +0800, Peng Sun wrote:
 Signed-off-by: Peng Sun sironhide0n...@gmail.com

Please always add a changelog message. Also, you need to fix the subject. The
convention is to use something like: a: b: ...
So in this case it should be something like: staging: android: ...
You can look at `git log drivers/staging/android` for inspiration.  This applies
to all of your other patches as well.  Finally, I don't think there is a point
in using deep threading in this case.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] staging: fbtft: Made into two lines

[Giedrius Statkevičius]

On Sun, Aug 23, 2015 at 07:00:23PM +0530, Aparna Karuthodi wrote:
> Oh! Sorry! I made the changes to correct the faults you figured out.
> Is it okay now?

Better. Also, submit it again with proper commit message and put "PATCH v2" in
the subject.

> 
> Signed-off-by: Aparna Karuthodi 
> ---
>  drivers/staging/fbtft/fb_pcd8544.c |2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/fbtft/fb_pcd8544.c 
> b/drivers/staging/fbtft/fb_pcd8544.c
> index cf87ce8..925511f 100644
> --- a/drivers/staging/fbtft/fb_pcd8544.c
> +++ b/drivers/staging/fbtft/fb_pcd8544.c
> @@ -34,8 +34,8 @@
>  #define WIDTH  84
>  #define HEIGHT 48
>  #define TXBUFLEN   (84*6)
> -#define DEFAULT_GAMMA  "40"
>  /* gamma is used to control contrast in this driver */

Aren't these two supposed to be on the same line? I think your patch is a bit
messed up. Doublecheck this.

> +#define DEFAULT_GAMMA  "40"
>  
>  static unsigned tc;
>  module_param(tc, uint, 0);
> -- 
> 1.7.9.5
> 
> ___
> devel mailing list
> de...@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] staging: fbtft: Made into two lines

[Giedrius Statkevičius]

On Sun, Aug 23, 2015 at 07:00:23PM +0530, Aparna Karuthodi wrote:
> Oh! Sorry! I made the changes to correct the faults you figured out.
> Is it okay now?

Resubmit it properly with "PATCH v2" in the title and a good commit message.

> 
> Signed-off-by: Aparna Karuthodi 
> ---
>  drivers/staging/fbtft/fb_pcd8544.c |2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/fbtft/fb_pcd8544.c 
> b/drivers/staging/fbtft/fb_pcd8544.c
> index cf87ce8..925511f 100644
> --- a/drivers/staging/fbtft/fb_pcd8544.c
> +++ b/drivers/staging/fbtft/fb_pcd8544.c
> @@ -34,8 +34,8 @@
>  #define WIDTH  84
>  #define HEIGHT 48
>  #define TXBUFLEN   (84*6)
> -#define DEFAULT_GAMMA  "40"
>  /* gamma is used to control contrast in this driver */

Isn't this comment supposed to be on the previous line (the removed line)?
Check if you haven't messed this up

> +#define DEFAULT_GAMMA  "40"
>  
>  static unsigned tc;
>  module_param(tc, uint, 0);
> -- 
> 1.7.9.5
> 
> ___
> devel mailing list
> de...@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] staging: fbtft: Made into two lines

[Giedrius Statkevičius]

On Sun, Aug 23, 2015 at 07:00:23PM +0530, Aparna Karuthodi wrote:
 Oh! Sorry! I made the changes to correct the faults you figured out.
 Is it okay now?

Resubmit it properly with PATCH v2 in the title and a good commit message.

 
 Signed-off-by: Aparna Karuthodi kdasapa...@gmail.com
 ---
  drivers/staging/fbtft/fb_pcd8544.c |2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/drivers/staging/fbtft/fb_pcd8544.c 
 b/drivers/staging/fbtft/fb_pcd8544.c
 index cf87ce8..925511f 100644
 --- a/drivers/staging/fbtft/fb_pcd8544.c
 +++ b/drivers/staging/fbtft/fb_pcd8544.c
 @@ -34,8 +34,8 @@
  #define WIDTH  84
  #define HEIGHT 48
  #define TXBUFLEN   (84*6)
 -#define DEFAULT_GAMMA  40
  /* gamma is used to control contrast in this driver */

Isn't this comment supposed to be on the previous line (the removed line)?
Check if you haven't messed this up

 +#define DEFAULT_GAMMA  40
  
  static unsigned tc;
  module_param(tc, uint, 0);
 -- 
 1.7.9.5
 
 ___
 devel mailing list
 de...@linuxdriverproject.org
 http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] staging: fbtft: Made into two lines

[Giedrius Statkevičius]

On Sun, Aug 23, 2015 at 07:00:23PM +0530, Aparna Karuthodi wrote:
 Oh! Sorry! I made the changes to correct the faults you figured out.
 Is it okay now?

Better. Also, submit it again with proper commit message and put PATCH v2 in
the subject.

 
 Signed-off-by: Aparna Karuthodi kdasapa...@gmail.com
 ---
  drivers/staging/fbtft/fb_pcd8544.c |2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/drivers/staging/fbtft/fb_pcd8544.c 
 b/drivers/staging/fbtft/fb_pcd8544.c
 index cf87ce8..925511f 100644
 --- a/drivers/staging/fbtft/fb_pcd8544.c
 +++ b/drivers/staging/fbtft/fb_pcd8544.c
 @@ -34,8 +34,8 @@
  #define WIDTH  84
  #define HEIGHT 48
  #define TXBUFLEN   (84*6)
 -#define DEFAULT_GAMMA  40
  /* gamma is used to control contrast in this driver */

Aren't these two supposed to be on the same line? I think your patch is a bit
messed up. Doublecheck this.

 +#define DEFAULT_GAMMA  40
  
  static unsigned tc;
  module_param(tc, uint, 0);
 -- 
 1.7.9.5
 
 ___
 devel mailing list
 de...@linuxdriverproject.org
 http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] staging: fbtft: Made into two lines

[Giedrius Statkevičius]

On Fri, Aug 21, 2015 at 06:38:39PM +0530, Aparna Karuthodi wrote:
> Made the comment into a new lineto remove a coding style error detected
> by checkpatch.
> The warning is given below:
> drivers/staging/fbtft/fb_pcd8544.c:37: WARNING: line over 80 characters
> 
> Signed-off-by: Aparna Karuthodi 
> ---
>  drivers/staging/fbtft/fb_pcd8544.c |3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/fbtft/fb_pcd8544.c 
> b/drivers/staging/fbtft/fb_pcd8544.c
> index 8b9ebfb..cf87ce8 100644
> --- a/drivers/staging/fbtft/fb_pcd8544.c
> +++ b/drivers/staging/fbtft/fb_pcd8544.c
> @@ -34,7 +34,8 @@
>  #define WIDTH  84
>  #define HEIGHT 48
>  #define TXBUFLEN   (84*6)
> -#define DEFAULT_GAMMA  "40" /* gamma is used to control contrast in this 
> driver */
> +#define DEFAULT_GAMMA  "40"
> +/* gamma is used to control contrast in this driver */

I think the format is usually:
/* comment */
#define FOO BAR

>  
>  static unsigned tc;
>  module_param(tc, uint, 0);
> -- 
> 1.7.9.5
> 
> ___
> devel mailing list
> de...@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] staging: fbtft: Made into two lines

[Giedrius Statkevičius]

On Fri, Aug 21, 2015 at 06:38:39PM +0530, Aparna Karuthodi wrote:
 Made the comment into a new lineto remove a coding style error detected
 by checkpatch.
 The warning is given below:
 drivers/staging/fbtft/fb_pcd8544.c:37: WARNING: line over 80 characters
 
 Signed-off-by: Aparna Karuthodi kdasapa...@gmail.com
 ---
  drivers/staging/fbtft/fb_pcd8544.c |3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)
 
 diff --git a/drivers/staging/fbtft/fb_pcd8544.c 
 b/drivers/staging/fbtft/fb_pcd8544.c
 index 8b9ebfb..cf87ce8 100644
 --- a/drivers/staging/fbtft/fb_pcd8544.c
 +++ b/drivers/staging/fbtft/fb_pcd8544.c
 @@ -34,7 +34,8 @@
  #define WIDTH  84
  #define HEIGHT 48
  #define TXBUFLEN   (84*6)
 -#define DEFAULT_GAMMA  40 /* gamma is used to control contrast in this 
 driver */
 +#define DEFAULT_GAMMA  40
 +/* gamma is used to control contrast in this driver */

I think the format is usually:
/* comment */
#define FOO BAR

  
  static unsigned tc;
  module_param(tc, uint, 0);
 -- 
 1.7.9.5
 
 ___
 devel mailing list
 de...@linuxdriverproject.org
 http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2] staging: ft1000: convert pack pragma to __packed

[Giedrius Statkevičius]

Convert a Microsoft compiler specific directive "#pragma pack(1)" to a
GCC equivalent __packed. Also, by doing this we save
ourselves from trouble if any other struct definitions are added after
the #pragma because it will be applied to all of the definitions
following it.

Signed-off-by: Giedrius Statkevičius 
---
v2: Convert __atribute__ ((__packed__)) to __packed as advised by Joe Perches
---
 drivers/staging/ft1000/ft1000-usb/ft1000_download.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/ft1000/ft1000-usb/ft1000_download.c 
b/drivers/staging/ft1000/ft1000-usb/ft1000_download.c
index 5def347..121792f 100644
--- a/drivers/staging/ft1000/ft1000-usb/ft1000_download.c
+++ b/drivers/staging/ft1000/ft1000-usb/ft1000_download.c
@@ -95,7 +95,6 @@ struct dsp_file_hdr {
long  nDspImages;  /* Number of DSP images in file. 
*/
 };
 
-#pragma pack(1)
 struct dsp_image_info {
long  coff_date;   /* Date/time when DSP Coff image 
was built. */
long  begin_offset;/* Offset in file where image 
begins. */
@@ -105,7 +104,7 @@ struct dsp_image_info {
long  version; /* Embedded version # of DSP 
code. */
unsigned shortchecksum;/* DSP File checksum */
unsigned shortpad1;
-};
+} __packed;
 
 
 /* checks if the doorbell register is cleared */
-- 
2.4.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2] staging: ft1000: convert pack pragma to __packed

[Giedrius Statkevičius]

Convert a Microsoft compiler specific directive #pragma pack(1) to a
GCC equivalent __packed. Also, by doing this we save
ourselves from trouble if any other struct definitions are added after
the #pragma because it will be applied to all of the definitions
following it.

Signed-off-by: Giedrius Statkevičius giedrius.statkevic...@gmail.com
---
v2: Convert __atribute__ ((__packed__)) to __packed as advised by Joe Perches
---
 drivers/staging/ft1000/ft1000-usb/ft1000_download.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/ft1000/ft1000-usb/ft1000_download.c 
b/drivers/staging/ft1000/ft1000-usb/ft1000_download.c
index 5def347..121792f 100644
--- a/drivers/staging/ft1000/ft1000-usb/ft1000_download.c
+++ b/drivers/staging/ft1000/ft1000-usb/ft1000_download.c
@@ -95,7 +95,6 @@ struct dsp_file_hdr {
long  nDspImages;  /* Number of DSP images in file. 
*/
 };
 
-#pragma pack(1)
 struct dsp_image_info {
long  coff_date;   /* Date/time when DSP Coff image 
was built. */
long  begin_offset;/* Offset in file where image 
begins. */
@@ -105,7 +104,7 @@ struct dsp_image_info {
long  version; /* Embedded version # of DSP 
code. */
unsigned shortchecksum;/* DSP File checksum */
unsigned shortpad1;
-};
+} __packed;
 
 
 /* checks if the doorbell register is cleared */
-- 
2.4.4

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] staging: ft1000: convert pack pragma to __packed__ attribute

[Giedrius Statkevičius]

Convert a Microsoft compiler specific directive "#pragma pack(1)" to a
GCC equivalent __attribute__ ((__packed__)). Also, by doing this we save
ourselves from trouble if any other struct definitions are added after
the #pragma because it will be applied to all of the definitions
following it.

Signed-off-by: Giedrius Statkevičius 
---
 drivers/staging/ft1000/ft1000-usb/ft1000_download.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/ft1000/ft1000-usb/ft1000_download.c 
b/drivers/staging/ft1000/ft1000-usb/ft1000_download.c
index 5def347..345c256 100644
--- a/drivers/staging/ft1000/ft1000-usb/ft1000_download.c
+++ b/drivers/staging/ft1000/ft1000-usb/ft1000_download.c
@@ -95,7 +95,6 @@ struct dsp_file_hdr {
long  nDspImages;  /* Number of DSP images in file. 
*/
 };
 
-#pragma pack(1)
 struct dsp_image_info {
long  coff_date;   /* Date/time when DSP Coff image 
was built. */
long  begin_offset;/* Offset in file where image 
begins. */
@@ -105,7 +104,7 @@ struct dsp_image_info {
long  version; /* Embedded version # of DSP 
code. */
unsigned shortchecksum;/* DSP File checksum */
unsigned shortpad1;
-};
+} __attribute__ ((__packed__));
 
 
 /* checks if the doorbell register is cleared */
-- 
2.4.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] staging: ft1000: convert pack pragma to __packed__ attribute

[Giedrius Statkevičius]

Convert a Microsoft compiler specific directive #pragma pack(1) to a
GCC equivalent __attribute__ ((__packed__)). Also, by doing this we save
ourselves from trouble if any other struct definitions are added after
the #pragma because it will be applied to all of the definitions
following it.

Signed-off-by: Giedrius Statkevičius giedrius.statkevic...@gmail.com
---
 drivers/staging/ft1000/ft1000-usb/ft1000_download.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/ft1000/ft1000-usb/ft1000_download.c 
b/drivers/staging/ft1000/ft1000-usb/ft1000_download.c
index 5def347..345c256 100644
--- a/drivers/staging/ft1000/ft1000-usb/ft1000_download.c
+++ b/drivers/staging/ft1000/ft1000-usb/ft1000_download.c
@@ -95,7 +95,6 @@ struct dsp_file_hdr {
long  nDspImages;  /* Number of DSP images in file. 
*/
 };
 
-#pragma pack(1)
 struct dsp_image_info {
long  coff_date;   /* Date/time when DSP Coff image 
was built. */
long  begin_offset;/* Offset in file where image 
begins. */
@@ -105,7 +104,7 @@ struct dsp_image_info {
long  version; /* Embedded version # of DSP 
code. */
unsigned shortchecksum;/* DSP File checksum */
unsigned shortpad1;
-};
+} __attribute__ ((__packed__));
 
 
 /* checks if the doorbell register is cleared */
-- 
2.4.4

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] mfd: rtsx: add support for rts522A

[Giedrius Statkevičius]

On Thu, 16 Apr 2015, micky_ch...@realsil.com.cn wrote:

> From: Micky Ching 
> 
> rts522a(rts5227s) is derived from rts5227, and mainly same with rts5227.
> Add it to file mfd/rts5227.c to support this chip.
> 
> Signed-off-by: Micky Ching 
> ---

Maybe update the Kconfig as well? So that the people who build the kernel would
know that this driver/module supports this chip

>  drivers/mfd/rts5227.c| 77 
> ++--
>  drivers/mfd/rtsx_pcr.c   |  5 +++
>  drivers/mfd/rtsx_pcr.h   |  3 ++
>  include/linux/mfd/rtsx_pci.h |  6 
>  4 files changed, 89 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mfd/rts5227.c b/drivers/mfd/rts5227.c
> index ce012d7..cf13e66 100644
> --- a/drivers/mfd/rts5227.c
> +++ b/drivers/mfd/rts5227.c
> @@ -26,6 +26,14 @@
>  
>  #include "rtsx_pcr.h"
>  
> +static u8 rts5227_get_ic_version(struct rtsx_pcr *pcr)
> +{
> + u8 val;
> +
> + rtsx_pci_read_register(pcr, DUMMY_REG_RESET_0, );
> + return val & 0x0F;
> +}
> +
>  static void rts5227_fill_driving(struct rtsx_pcr *pcr, u8 voltage)
>  {
>   u8 driving_3v3[4][3] = {
> @@ -88,7 +96,7 @@ static void rts5227_force_power_down(struct rtsx_pcr *pcr, 
> u8 pm_state)
>   rtsx_pci_write_register(pcr, AUTOLOAD_CFG_BASE + 3, 0x01, 0);
>  
>   if (pm_state == HOST_ENTER_S3)
> - rtsx_pci_write_register(pcr, PM_CTRL3, 0x10, 0x10);
> + rtsx_pci_write_register(pcr, pcr->reg_pm_ctrl3, 0x10, 0x10);
>  
>   rtsx_pci_write_register(pcr, FPDCTL, 0x03, 0x03);
>  }
> @@ -121,7 +129,7 @@ static int rts5227_extra_init_hw(struct rtsx_pcr *pcr)
>   rtsx_pci_add_cmd(pcr, WRITE_REG_CMD, PETXCFG, 0xB8, 0xB8);
>   else
>   rtsx_pci_add_cmd(pcr, WRITE_REG_CMD, PETXCFG, 0xB8, 0x88);
> - rtsx_pci_add_cmd(pcr, WRITE_REG_CMD, PM_CTRL3, 0x10, 0x00);
> + rtsx_pci_add_cmd(pcr, WRITE_REG_CMD, pcr->reg_pm_ctrl3, 0x10, 0x00);
>  
>   return rtsx_pci_send_cmd(pcr, 100);
>  }
> @@ -298,8 +306,73 @@ void rts5227_init_params(struct rtsx_pcr *pcr)
>   pcr->tx_initial_phase = SET_CLOCK_PHASE(27, 27, 15);
>   pcr->rx_initial_phase = SET_CLOCK_PHASE(30, 7, 7);
>  
> + pcr->ic_version = rts5227_get_ic_version(pcr);
>   pcr->sd_pull_ctl_enable_tbl = rts5227_sd_pull_ctl_enable_tbl;
>   pcr->sd_pull_ctl_disable_tbl = rts5227_sd_pull_ctl_disable_tbl;
>   pcr->ms_pull_ctl_enable_tbl = rts5227_ms_pull_ctl_enable_tbl;
>   pcr->ms_pull_ctl_disable_tbl = rts5227_ms_pull_ctl_disable_tbl;
> +
> + pcr->reg_pm_ctrl3 = PM_CTRL3;
> +}
> +
> +static int rts522a_optimize_phy(struct rtsx_pcr *pcr)
> +{
> + int err;
> +
> + err = rtsx_pci_write_register(pcr, RTS522A_PM_CTRL3, D3_DELINK_MODE_EN,
> + 0x00);
> + if (err < 0)
> + return err;
> +
> + if (is_version(pcr, 0x522A, IC_VER_A)) {
> + err = rtsx_pci_write_phy_register(pcr, PHY_RCR2,
> + PHY_RCR2_INIT_27S);
> + if (err)
> + return err;
> +
> + rtsx_pci_write_phy_register(pcr, PHY_RCR1, PHY_RCR1_INIT_27S);
> + rtsx_pci_write_phy_register(pcr, PHY_FLD0, PHY_FLD0_INIT_27S);
> + rtsx_pci_write_phy_register(pcr, PHY_FLD3, PHY_FLD3_INIT_27S);
> + rtsx_pci_write_phy_register(pcr, PHY_FLD4, PHY_FLD4_INIT_27S);
> + }
> +
> + return 0;
> +}
> +
> +static int rts522a_extra_init_hw(struct rtsx_pcr *pcr)
> +{
> + rts5227_extra_init_hw(pcr);
> +
> + rtsx_pci_write_register(pcr, FUNC_FORCE_CTL, FUNC_FORCE_UPME_XMT_DBG,
> + FUNC_FORCE_UPME_XMT_DBG);
> + rtsx_pci_write_register(pcr, PCLK_CTL, 0x04, 0x04);
> + rtsx_pci_write_register(pcr, PM_EVENT_DEBUG, PME_DEBUG_0, PME_DEBUG_0);
> + rtsx_pci_write_register(pcr, PM_CLK_FORCE_CTL, 0xFF, 0x11);
> +
> + return 0;
> +}
> +
> +/* rts522a operations mainly derived from rts5227, except phy/hw init 
> setting.
> + */
> +static const struct pcr_ops rts522a_pcr_ops = {
> + .fetch_vendor_settings = rts5227_fetch_vendor_settings,
> + .extra_init_hw = rts522a_extra_init_hw,
> + .optimize_phy = rts522a_optimize_phy,
> + .turn_on_led = rts5227_turn_on_led,
> + .turn_off_led = rts5227_turn_off_led,
> + .enable_auto_blink = rts5227_enable_auto_blink,
> + .disable_auto_blink = rts5227_disable_auto_blink,
> + .card_power_on = rts5227_card_power_on,
> + .card_power_off = rts5227_card_power_off,
> + .switch_output_voltage = rts5227_switch_output_voltage,
> + .cd_deglitch = NULL,
> + .conv_clk_and_div_n = NULL,
> + .force_power_down = rts5227_force_power_down,
> +};
> +
> +void rts522a_init_params(struct rtsx_pcr *pcr)
> +{
> + rts5227_init_params(pcr);
> +
> + pcr->reg_pm_ctrl3 = RTS522A_PM_CTRL3;
>  }
> diff --git a/drivers/mfd/rtsx_pcr.c b/drivers/mfd/rtsx_pcr.c
> index a66540a..ccd8918 100644
> --- a/drivers/mfd/rtsx_pcr.c
> +++ b/drivers/mfd/rtsx_pcr.c
> @@ -55,6 +55,7 @@ static const struct 

Re: [PATCH] mfd: rtsx: add support for rts522A

[Giedrius Statkevičius]

On Thu, 16 Apr 2015, micky_ch...@realsil.com.cn wrote:

 From: Micky Ching micky_ch...@realsil.com.cn
 
 rts522a(rts5227s) is derived from rts5227, and mainly same with rts5227.
 Add it to file mfd/rts5227.c to support this chip.
 
 Signed-off-by: Micky Ching micky_ch...@realsil.com.cn
 ---

Maybe update the Kconfig as well? So that the people who build the kernel would
know that this driver/module supports this chip

  drivers/mfd/rts5227.c| 77 
 ++--
  drivers/mfd/rtsx_pcr.c   |  5 +++
  drivers/mfd/rtsx_pcr.h   |  3 ++
  include/linux/mfd/rtsx_pci.h |  6 
  4 files changed, 89 insertions(+), 2 deletions(-)
 
 diff --git a/drivers/mfd/rts5227.c b/drivers/mfd/rts5227.c
 index ce012d7..cf13e66 100644
 --- a/drivers/mfd/rts5227.c
 +++ b/drivers/mfd/rts5227.c
 @@ -26,6 +26,14 @@
  
  #include rtsx_pcr.h
  
 +static u8 rts5227_get_ic_version(struct rtsx_pcr *pcr)
 +{
 + u8 val;
 +
 + rtsx_pci_read_register(pcr, DUMMY_REG_RESET_0, val);
 + return val  0x0F;
 +}
 +
  static void rts5227_fill_driving(struct rtsx_pcr *pcr, u8 voltage)
  {
   u8 driving_3v3[4][3] = {
 @@ -88,7 +96,7 @@ static void rts5227_force_power_down(struct rtsx_pcr *pcr, 
 u8 pm_state)
   rtsx_pci_write_register(pcr, AUTOLOAD_CFG_BASE + 3, 0x01, 0);
  
   if (pm_state == HOST_ENTER_S3)
 - rtsx_pci_write_register(pcr, PM_CTRL3, 0x10, 0x10);
 + rtsx_pci_write_register(pcr, pcr-reg_pm_ctrl3, 0x10, 0x10);
  
   rtsx_pci_write_register(pcr, FPDCTL, 0x03, 0x03);
  }
 @@ -121,7 +129,7 @@ static int rts5227_extra_init_hw(struct rtsx_pcr *pcr)
   rtsx_pci_add_cmd(pcr, WRITE_REG_CMD, PETXCFG, 0xB8, 0xB8);
   else
   rtsx_pci_add_cmd(pcr, WRITE_REG_CMD, PETXCFG, 0xB8, 0x88);
 - rtsx_pci_add_cmd(pcr, WRITE_REG_CMD, PM_CTRL3, 0x10, 0x00);
 + rtsx_pci_add_cmd(pcr, WRITE_REG_CMD, pcr-reg_pm_ctrl3, 0x10, 0x00);
  
   return rtsx_pci_send_cmd(pcr, 100);
  }
 @@ -298,8 +306,73 @@ void rts5227_init_params(struct rtsx_pcr *pcr)
   pcr-tx_initial_phase = SET_CLOCK_PHASE(27, 27, 15);
   pcr-rx_initial_phase = SET_CLOCK_PHASE(30, 7, 7);
  
 + pcr-ic_version = rts5227_get_ic_version(pcr);
   pcr-sd_pull_ctl_enable_tbl = rts5227_sd_pull_ctl_enable_tbl;
   pcr-sd_pull_ctl_disable_tbl = rts5227_sd_pull_ctl_disable_tbl;
   pcr-ms_pull_ctl_enable_tbl = rts5227_ms_pull_ctl_enable_tbl;
   pcr-ms_pull_ctl_disable_tbl = rts5227_ms_pull_ctl_disable_tbl;
 +
 + pcr-reg_pm_ctrl3 = PM_CTRL3;
 +}
 +
 +static int rts522a_optimize_phy(struct rtsx_pcr *pcr)
 +{
 + int err;
 +
 + err = rtsx_pci_write_register(pcr, RTS522A_PM_CTRL3, D3_DELINK_MODE_EN,
 + 0x00);
 + if (err  0)
 + return err;
 +
 + if (is_version(pcr, 0x522A, IC_VER_A)) {
 + err = rtsx_pci_write_phy_register(pcr, PHY_RCR2,
 + PHY_RCR2_INIT_27S);
 + if (err)
 + return err;
 +
 + rtsx_pci_write_phy_register(pcr, PHY_RCR1, PHY_RCR1_INIT_27S);
 + rtsx_pci_write_phy_register(pcr, PHY_FLD0, PHY_FLD0_INIT_27S);
 + rtsx_pci_write_phy_register(pcr, PHY_FLD3, PHY_FLD3_INIT_27S);
 + rtsx_pci_write_phy_register(pcr, PHY_FLD4, PHY_FLD4_INIT_27S);
 + }
 +
 + return 0;
 +}
 +
 +static int rts522a_extra_init_hw(struct rtsx_pcr *pcr)
 +{
 + rts5227_extra_init_hw(pcr);
 +
 + rtsx_pci_write_register(pcr, FUNC_FORCE_CTL, FUNC_FORCE_UPME_XMT_DBG,
 + FUNC_FORCE_UPME_XMT_DBG);
 + rtsx_pci_write_register(pcr, PCLK_CTL, 0x04, 0x04);
 + rtsx_pci_write_register(pcr, PM_EVENT_DEBUG, PME_DEBUG_0, PME_DEBUG_0);
 + rtsx_pci_write_register(pcr, PM_CLK_FORCE_CTL, 0xFF, 0x11);
 +
 + return 0;
 +}
 +
 +/* rts522a operations mainly derived from rts5227, except phy/hw init 
 setting.
 + */
 +static const struct pcr_ops rts522a_pcr_ops = {
 + .fetch_vendor_settings = rts5227_fetch_vendor_settings,
 + .extra_init_hw = rts522a_extra_init_hw,
 + .optimize_phy = rts522a_optimize_phy,
 + .turn_on_led = rts5227_turn_on_led,
 + .turn_off_led = rts5227_turn_off_led,
 + .enable_auto_blink = rts5227_enable_auto_blink,
 + .disable_auto_blink = rts5227_disable_auto_blink,
 + .card_power_on = rts5227_card_power_on,
 + .card_power_off = rts5227_card_power_off,
 + .switch_output_voltage = rts5227_switch_output_voltage,
 + .cd_deglitch = NULL,
 + .conv_clk_and_div_n = NULL,
 + .force_power_down = rts5227_force_power_down,
 +};
 +
 +void rts522a_init_params(struct rtsx_pcr *pcr)
 +{
 + rts5227_init_params(pcr);
 +
 + pcr-reg_pm_ctrl3 = RTS522A_PM_CTRL3;
  }
 diff --git a/drivers/mfd/rtsx_pcr.c b/drivers/mfd/rtsx_pcr.c
 index a66540a..ccd8918 100644
 --- a/drivers/mfd/rtsx_pcr.c
 +++ b/drivers/mfd/rtsx_pcr.c
 @@ -55,6 +55,7 @@ static const struct pci_device_id rtsx_pci_ids[] = {
   { PCI_DEVICE(0x10EC, 0x5229), PCI_CLASS_OTHERS  16, 

Re: [PATCH v2 1/2 RESEND] staging: dgnc: remove dead code in dgnc_tty_write()

[Giedrius Statkevičius]

On Sat, 11 Apr 2015, Sudip Mukherjee wrote:

> On Fri, Apr 10, 2015 at 05:48:54PM +0300, Giedrius Statkevičius wrote:
> > Remove the dead code protected by in_user in dgnc_tty_write() because it is 
> > set
> > to 0 and never changed to 1 thus the code in ifs never gets executed.
> dgnc_tty_write() is being called by dgnc_tty_put_char() and it is also
> the write callback function of struct tty_operations, so I think the
> correct fix will be to use from_user and make it 0 when
> dgnc_tty_put_char() calls this function else make it 1 to inform the
> function that the data wil be coming from the userspace. Maybe some
> thing like this:
Well, I think this is wrong because:
* parameter of write member of struct tty_operations buf is not tagged with
  "__user" so it should be safe to just memcpy() from it
* Looked through some other write operations in other tty drivers and I've never
  seen copy_from_user() used on buf argument of write operation - always
  memcpy() or some other similar function
* Ldd3 (and the comments in tty_driver.h) says that write could be called from
  interrupt context too so it can't sleep and thus use copy_from_user()

While looking through I've caught that dgnc_TmpWriteBuf could be also deleted
because it will be unused if this patch goes through. If it does then I'll send
another one (or should I send a v3?)

Su pagarba / Regards,
Giedrius

Re: [PATCH v2 1/2 RESEND] staging: dgnc: remove dead code in dgnc_tty_write()

[Giedrius Statkevičius]

On Sat, 11 Apr 2015, Sudip Mukherjee wrote:

 On Fri, Apr 10, 2015 at 05:48:54PM +0300, Giedrius Statkevičius wrote:
  Remove the dead code protected by in_user in dgnc_tty_write() because it is 
  set
  to 0 and never changed to 1 thus the code in ifs never gets executed.
 dgnc_tty_write() is being called by dgnc_tty_put_char() and it is also
 the write callback function of struct tty_operations, so I think the
 correct fix will be to use from_user and make it 0 when
 dgnc_tty_put_char() calls this function else make it 1 to inform the
 function that the data wil be coming from the userspace. Maybe some
 thing like this:
Well, I think this is wrong because:
* parameter of write member of struct tty_operations buf is not tagged with
  __user so it should be safe to just memcpy() from it
* Looked through some other write operations in other tty drivers and I've never
  seen copy_from_user() used on buf argument of write operation - always
  memcpy() or some other similar function
* Ldd3 (and the comments in tty_driver.h) says that write could be called from
  interrupt context too so it can't sleep and thus use copy_from_user()

While looking through I've caught that dgnc_TmpWriteBuf could be also deleted
because it will be unused if this patch goes through. If it does then I'll send
another one (or should I send a v3?)

Su pagarba / Regards,
Giedrius

[PATCH] staging: dgnc: remove some dead code from dgnc_tty.c

[Giedrius Statkevičius]

Remove some dead code that will never be executed or which serves no purpose

Signed-off-by: Giedrius Statkevičius 
---
 drivers/staging/dgnc/dgnc_tty.c | 16 
 1 file changed, 16 deletions(-)

diff --git a/drivers/staging/dgnc/dgnc_tty.c b/drivers/staging/dgnc/dgnc_tty.c
index ce4187f..f954228 100644
--- a/drivers/staging/dgnc/dgnc_tty.c
+++ b/drivers/staging/dgnc/dgnc_tty.c
@@ -428,9 +428,6 @@ void dgnc_tty_uninit(struct dgnc_board *brd)
brd->PrintDriver.ttys = NULL;
 }
 
-
-#define TMPBUFLEN (1024)
-
 /*===
  *
  * dgnc_wmove - Write data to transmit queue.
@@ -555,15 +552,6 @@ void dgnc_input(struct channel_t *ch)
 
ld = tty_ldisc_ref(tp);
 
-#ifdef TTY_DONT_FLIP
-   /*
-* If the DONT_FLIP flag is on, don't flush our buffer, and act
-* like the ld doesn't have any space to put the data right now.
-*/
-   if (test_bit(TTY_DONT_FLIP, >flags))
-   len = 0;
-#endif
-
/*
 * If we were unable to get a reference to the ld,
 * don't flush our buffer, and act like the ld doesn't
@@ -897,10 +885,6 @@ void dgnc_check_queue_flow_control(struct channel_t *ch)
ch->ch_stops_sent = 0;
ch->ch_bd->bd_ops->send_start_character(ch);
}
-   /* No FLOW */
-   else {
-   /* Nothing needed. */
-   }
}
 }
 
-- 
2.3.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] staging: dgnc: remove some dead code from dgnc_tty.c

[Giedrius Statkevičius]

Remove some dead code that will never be executed or which serves no purpose

Signed-off-by: Giedrius Statkevičius giedrius.statkevic...@gmail.com
---
 drivers/staging/dgnc/dgnc_tty.c | 16 
 1 file changed, 16 deletions(-)

diff --git a/drivers/staging/dgnc/dgnc_tty.c b/drivers/staging/dgnc/dgnc_tty.c
index ce4187f..f954228 100644
--- a/drivers/staging/dgnc/dgnc_tty.c
+++ b/drivers/staging/dgnc/dgnc_tty.c
@@ -428,9 +428,6 @@ void dgnc_tty_uninit(struct dgnc_board *brd)
brd-PrintDriver.ttys = NULL;
 }
 
-
-#define TMPBUFLEN (1024)
-
 /*===
  *
  * dgnc_wmove - Write data to transmit queue.
@@ -555,15 +552,6 @@ void dgnc_input(struct channel_t *ch)
 
ld = tty_ldisc_ref(tp);
 
-#ifdef TTY_DONT_FLIP
-   /*
-* If the DONT_FLIP flag is on, don't flush our buffer, and act
-* like the ld doesn't have any space to put the data right now.
-*/
-   if (test_bit(TTY_DONT_FLIP, tp-flags))
-   len = 0;
-#endif
-
/*
 * If we were unable to get a reference to the ld,
 * don't flush our buffer, and act like the ld doesn't
@@ -897,10 +885,6 @@ void dgnc_check_queue_flow_control(struct channel_t *ch)
ch-ch_stops_sent = 0;
ch-ch_bd-bd_ops-send_start_character(ch);
}
-   /* No FLOW */
-   else {
-   /* Nothing needed. */
-   }
}
 }
 
-- 
2.3.5

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v4 1/3] staging: dgnc: clean up allocation of ->channels[i]

[Giedrius Statkevičius]

Check if kzalloc fails in dgnc_tty_init() and if it does then free all
previously allocated ->channels[i] and set them to NULL. This makes the code
less error/bug prone because instead of needing programmers attention to add
checks everywhere we do that in one place. Also, remove a bogus comment and
check in the same loop because ->channels[i] isn't allocated anywhere else.
Finally, remove a unnecessary check if ->channels[i] is NULL in the next loop
because it can't be.

Signed-off-by: Giedrius Statkevičius 
---
v4: Make this patch only for dgnc_tty.c and only for this thing. Split into 3
patches (1 patch fixes a bug reported by Dan Carpenter)

v3: Remove the wrong comment at dgnc_tty_init() that says the allocation could
happen somewhere else before this and remove the check if (!brd->channels[i]).
Also, remove all checks where ->channels[i] is checked if it's NULL or not
because if probe fails then that means that this scenario isn't possible (except
in the ioctl).

v2: Only returning -ENOMEM if an allocation failed isn't enough as it was
spotted by Sudip so create a new label that frees all successfully allocated
stuff and only then returns -ENOMEM. Also, remove a unnecessary check in the
next loop.

 drivers/staging/dgnc/dgnc_tty.c | 31 +++
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/drivers/staging/dgnc/dgnc_tty.c b/drivers/staging/dgnc/dgnc_tty.c
index ce4187f..0e48f95 100644
--- a/drivers/staging/dgnc/dgnc_tty.c
+++ b/drivers/staging/dgnc/dgnc_tty.c
@@ -304,19 +304,15 @@ int dgnc_tty_init(struct dgnc_board *brd)
 
brd->nasync = brd->maxports;
 
-   /*
-* Allocate channel memory that might not have been allocated
-* when the driver was first loaded.
-*/
for (i = 0; i < brd->nasync; i++) {
-   if (!brd->channels[i]) {
-
-   /*
-* Okay to malloc with GFP_KERNEL, we are not at
-* interrupt context, and there are no locks held.
-*/
-   brd->channels[i] = kzalloc(sizeof(*brd->channels[i]), 
GFP_KERNEL);
-   }
+   /*
+* Okay to malloc with GFP_KERNEL, we are not at
+* interrupt context, and there are no locks held.
+*/
+   brd->channels[i] = kzalloc(sizeof(*brd->channels[i]),
+  GFP_KERNEL);
+   if (!brd->channels[i])
+   goto err_free_channels;
}
 
ch = brd->channels[0];
@@ -324,10 +320,6 @@ int dgnc_tty_init(struct dgnc_board *brd)
 
/* Set up channel variables */
for (i = 0; i < brd->nasync; i++, ch = brd->channels[i]) {
-
-   if (!brd->channels[i])
-   continue;
-
spin_lock_init(>ch_lock);
 
/* Store all our magic numbers */
@@ -375,6 +367,13 @@ int dgnc_tty_init(struct dgnc_board *brd)
}
 
return 0;
+
+err_free_channels:
+   for (i = i - 1; i >= 0; --i) {
+   kfree(brd->channels[i]);
+   brd->channels[i] = NULL;
+   }
+   return -ENOMEM;
 }
 
 
-- 
2.3.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v4 2/3] staging: dgnc: don't forget to check if ->channels[i] is NULL in dgnc_tty_uninit()

[Giedrius Statkevičius]

Add a check if ->channels[i] is NULL because a NULL pointer may be dereferenced
in case one of the allocations failed

Reported-by: Dan Carpenter 
Signed-off-by: Giedrius Statkevičius 
---
v4: new patch that fixes a bug reported by Dan Carpenter

 drivers/staging/dgnc/dgnc_tty.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/dgnc/dgnc_tty.c b/drivers/staging/dgnc/dgnc_tty.c
index 0e48f95..785eb6c 100644
--- a/drivers/staging/dgnc/dgnc_tty.c
+++ b/drivers/staging/dgnc/dgnc_tty.c
@@ -403,7 +403,9 @@ void dgnc_tty_uninit(struct dgnc_board *brd)
dgnc_BoardsByMajor[brd->SerialDriver.major] = NULL;
brd->dgnc_Serial_Major = 0;
for (i = 0; i < brd->nasync; i++) {
-   
dgnc_remove_tty_sysfs(brd->channels[i]->ch_tun.un_sysfs);
+   if (brd->channels[i])
+   dgnc_remove_tty_sysfs(brd->channels[i]->
+ ch_tun.un_sysfs);
tty_unregister_device(>SerialDriver, i);
}
tty_unregister_driver(>SerialDriver);
@@ -414,7 +416,9 @@ void dgnc_tty_uninit(struct dgnc_board *brd)
dgnc_BoardsByMajor[brd->PrintDriver.major] = NULL;
brd->dgnc_TransparentPrint_Major = 0;
for (i = 0; i < brd->nasync; i++) {
-   
dgnc_remove_tty_sysfs(brd->channels[i]->ch_pun.un_sysfs);
+   if (brd->channels[i])
+   dgnc_remove_tty_sysfs(brd->channels[i]->
+ ch_pun.un_sysfs);
tty_unregister_device(>PrintDriver, i);
}
tty_unregister_driver(>PrintDriver);
-- 
2.3.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v4 3/3] staging: dgnc: remove redundant !ch checks

[Giedrius Statkevičius]

Remove checks that are redundant since we don't have boards with partially
initialized ->channels[i].

Signed-off-by: Giedrius Statkevičius 
---
v4: splitted this from the one patch.

 drivers/staging/dgnc/dgnc_cls.c | 4 ++--
 drivers/staging/dgnc/dgnc_neo.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/dgnc/dgnc_cls.c b/drivers/staging/dgnc/dgnc_cls.c
index e3564d2..82e8680 100644
--- a/drivers/staging/dgnc/dgnc_cls.c
+++ b/drivers/staging/dgnc/dgnc_cls.c
@@ -379,7 +379,7 @@ static inline void cls_parse_isr(struct dgnc_board *brd, 
uint port)
return;
 
ch = brd->channels[port];
-   if (!ch || ch->magic != DGNC_CHANNEL_MAGIC)
+   if (ch->magic != DGNC_CHANNEL_MAGIC)
return;
 
/* Here we try to figure out what caused the interrupt to happen */
@@ -691,7 +691,7 @@ static void cls_tasklet(unsigned long data)
int state = 0;
int ports = 0;
 
-   if (!bd || bd->magic != DGNC_BOARD_MAGIC)
+   if (bd->magic != DGNC_BOARD_MAGIC)
return;
 
/* Cache a couple board values */
diff --git a/drivers/staging/dgnc/dgnc_neo.c b/drivers/staging/dgnc/dgnc_neo.c
index f5a4d36..1e583c2 100644
--- a/drivers/staging/dgnc/dgnc_neo.c
+++ b/drivers/staging/dgnc/dgnc_neo.c
@@ -395,7 +395,7 @@ static inline void neo_parse_isr(struct dgnc_board *brd, 
uint port)
return;
 
ch = brd->channels[port];
-   if (!ch || ch->magic != DGNC_CHANNEL_MAGIC)
+   if (ch->magic != DGNC_CHANNEL_MAGIC)
return;
 
/* Here we try to figure out what caused the interrupt to happen */
-- 
2.3.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] staging: dgnc: check if kzalloc fails in dgnc_tty_init()

[Giedrius Statkevičius]

On Tue, 7 Apr 2015, Dan Carpenter wrote:

> You will need to update the subject to reflect the new patch.
> 
> The original code did check for kzalloc() failure but it had lots of
> checks scattered around instead nicely at the point where the memory
> was allocated.
> 

There are a lot missing too. For example in dgnc_sysfs.c there are no checks on
any _show() methods if ->channels[i] is NULL or not. And in some other places.

> The old code and the new code are both buggy though and will crash in
> dgnc_tty_uninit().  dgnc_found_board() does "One Err" style error
> handling so it's obviously buggy like the underside of a rock.
> https://plus.google.com/106378716002406849458/posts/dnanfhQ4mHQ
> 
> It's becoming a difficult thing to fix this because every time we look
> there are more things which don't make sense.
> 
> I believe that if you do:
> 
> > +err_free_channels:
> > +   for (i = i - 1; i >= 0; --i) {
> > +   kfree(brd->channels[i]);
>   brd->channels[i] = NULL;
>   }
> > +   return -ENOMEM;
> >  }
> 
> And add some NULL checks in dgnc_tty_uninit() to see if ->channels[i] is
> NULL before doing:
> 
>   dgnc_remove_tty_sysfs(brd->channels[i]->ch_tun.un_sysfs);
> 
> and
>   dgnc_remove_tty_sysfs(brd->channels[i]->ch_pun.un_sysfs);
> 
> Then it will fix the bug.
> 

Missed this. Yep, I agree. 

> Do these in a separate patch.  I'm looking for ways we can make this
> patch minimal.  Deleting the comments and the NULL check in
> dgnc_tty_init() is essential for the patch because otherwise the cleanup
> doesn't make sense.

Well, the point of the patch is to put alloc and checks in one place to make the
code less error and bug prone and fix some of bugs where ->channels[i] isn't
checked. Ok, I'll send a v5 that's split into two patches.

> 
> regards,
> dan carpenter
> 
> 

Su pagarba / Regards,
Giedrius
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3] staging: dgnc: check if kzalloc fails in dgnc_tty_init()

[Giedrius Statkevičius]

On Tue, 7 Apr 2015, Sudip Mukherjee wrote:

> On Tue, Apr 07, 2015 at 05:11:15PM +0300, Giedrius Statkevičius wrote:
> > If one of the allocations of memory for storing a channel information struct
> > fails then free all the successful allocations and return -ENOMEM that gets
> > propogated to the pci layer.  Also, remove a bogus skipping in the next 
> > part of
> > the initiation if a previous memory allocation failed because we won't 
> > execute
> > that if any of the allocations failed. Next, remove the misleading comment 
> > that
> > allocation could happen elsewhere. Finally, remove all (except in the ioctl
> > which can try to get information about a board that failed to probe) checks 
> > if
> > ->channels[foo] is NULL or not because probe failing if we can't allocate 
> > enough
> > memory means that this scenario isn't possible.
> 
> i think now it became too many changes for a single patch..
> 
> regards
> sudip

If I split this into two patches then they both would have to be applied and
Greg or someone else could miss that one patch depended on another and leave the
kernel in a partial state so I think it's best to keep it in one. Lets see what
other people have to say.

Su pagarba / Regards,
Giedrius

[PATCH v3] staging: dgnc: check if kzalloc fails in dgnc_tty_init()

[Giedrius Statkevičius]

If one of the allocations of memory for storing a channel information struct
fails then free all the successful allocations and return -ENOMEM that gets
propogated to the pci layer.  Also, remove a bogus skipping in the next part of
the initiation if a previous memory allocation failed because we won't execute
that if any of the allocations failed. Next, remove the misleading comment that
allocation could happen elsewhere. Finally, remove all (except in the ioctl
which can try to get information about a board that failed to probe) checks if
->channels[foo] is NULL or not because probe failing if we can't allocate enough
memory means that this scenario isn't possible.

Signed-off-by: Giedrius Statkevičius 
---
v3: Remove the wrong comment at dgnc_tty_init() that says the allocation could
happen somewhere else before this and remove the check if (!brd->channels[i]).
Also, remove all checks where ->channels[i] is checked if it's NULL or not
because if probe fails then that means that this scenario isn't possible (except
in the ioctl).

v2: Only returning -ENOMEM if an allocation failed isn't enough as it was
spotted by Sudip so create a new label that frees all successfully allocated
stuff and only then returns -ENOMEM. Also, remove a unnecessary check in the
next loop.

 drivers/staging/dgnc/dgnc_cls.c |  4 +---
 drivers/staging/dgnc/dgnc_neo.c |  2 +-
 drivers/staging/dgnc/dgnc_tty.c | 29 +
 3 files changed, 15 insertions(+), 20 deletions(-)

diff --git a/drivers/staging/dgnc/dgnc_cls.c b/drivers/staging/dgnc/dgnc_cls.c
index e3564d2..a629a78 100644
--- a/drivers/staging/dgnc/dgnc_cls.c
+++ b/drivers/staging/dgnc/dgnc_cls.c
@@ -379,7 +379,7 @@ static inline void cls_parse_isr(struct dgnc_board *brd, 
uint port)
return;
 
ch = brd->channels[port];
-   if (!ch || ch->magic != DGNC_CHANNEL_MAGIC)
+   if (ch->magic != DGNC_CHANNEL_MAGIC)
return;
 
/* Here we try to figure out what caused the interrupt to happen */
@@ -714,8 +714,6 @@ static void cls_tasklet(unsigned long data)
/* Loop on each port */
for (i = 0; i < ports; i++) {
ch = bd->channels[i];
-   if (!ch)
-   continue;
 
/*
 * NOTE: Remember you CANNOT hold any channel
diff --git a/drivers/staging/dgnc/dgnc_neo.c b/drivers/staging/dgnc/dgnc_neo.c
index f5a4d36..1e583c2 100644
--- a/drivers/staging/dgnc/dgnc_neo.c
+++ b/drivers/staging/dgnc/dgnc_neo.c
@@ -395,7 +395,7 @@ static inline void neo_parse_isr(struct dgnc_board *brd, 
uint port)
return;
 
ch = brd->channels[port];
-   if (!ch || ch->magic != DGNC_CHANNEL_MAGIC)
+   if (ch->magic != DGNC_CHANNEL_MAGIC)
return;
 
/* Here we try to figure out what caused the interrupt to happen */
diff --git a/drivers/staging/dgnc/dgnc_tty.c b/drivers/staging/dgnc/dgnc_tty.c
index ce4187f..d1a2c0f 100644
--- a/drivers/staging/dgnc/dgnc_tty.c
+++ b/drivers/staging/dgnc/dgnc_tty.c
@@ -304,19 +304,15 @@ int dgnc_tty_init(struct dgnc_board *brd)
 
brd->nasync = brd->maxports;
 
-   /*
-* Allocate channel memory that might not have been allocated
-* when the driver was first loaded.
-*/
for (i = 0; i < brd->nasync; i++) {
-   if (!brd->channels[i]) {
-
-   /*
-* Okay to malloc with GFP_KERNEL, we are not at
-* interrupt context, and there are no locks held.
-*/
-   brd->channels[i] = kzalloc(sizeof(*brd->channels[i]), 
GFP_KERNEL);
-   }
+   /*
+* Okay to malloc with GFP_KERNEL, we are not at
+* interrupt context, and there are no locks held.
+*/
+   brd->channels[i] = kzalloc(sizeof(*brd->channels[i]),
+  GFP_KERNEL);
+   if (!brd->channels[i])
+   goto err_free_channels;
}
 
ch = brd->channels[0];
@@ -324,10 +320,6 @@ int dgnc_tty_init(struct dgnc_board *brd)
 
/* Set up channel variables */
for (i = 0; i < brd->nasync; i++, ch = brd->channels[i]) {
-
-   if (!brd->channels[i])
-   continue;
-
spin_lock_init(>ch_lock);
 
/* Store all our magic numbers */
@@ -375,6 +367,11 @@ int dgnc_tty_init(struct dgnc_board *brd)
}
 
return 0;
+
+err_free_channels:
+   for (i = i - 1; i >= 0; --i)
+   kfree(brd->channels[i]);
+   return -ENOMEM;
 }
 
 
-- 
2.3.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.or

Re: [PATCH v2] staging: dgnc: check if kzalloc fails in dgnc_tty_init()

[Giedrius Statkevičius]

On Tue, 7 Apr 2015, Dan Carpenter wrote:

> On Tue, Apr 07, 2015 at 03:40:17PM +0300, Giedrius Statkevičius wrote:
> > If one of the allocations of memory for storing a channel information struct
> > fails then free all the successful allocations and return -ENOMEM that gets
> > propogated to the pci layer.  Also, remove a bogus skipping in the next 
> > part of
> > the initiation if a previous memory allocation failed because we won't 
> > execute
> > that if any of the allocations failed.
> > 
> > Signed-off-by: Giedrius Statkevičius 
> > ---
> > v2: Only returning -ENOMEM if an allocation failed isn't enough as it was
> > spotted by Sudip so create a new label that frees all successfully allocated
> > stuff and only then returns -ENOMEM. Also, remove a unnecessary check in the
> > next loop.
> > 
> >  drivers/staging/dgnc/dgnc_tty.c | 11 +++
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/staging/dgnc/dgnc_tty.c 
> > b/drivers/staging/dgnc/dgnc_tty.c
> > index ce4187f..60d7e49 100644
> > --- a/drivers/staging/dgnc/dgnc_tty.c
> > +++ b/drivers/staging/dgnc/dgnc_tty.c
> > @@ -316,6 +316,8 @@ int dgnc_tty_init(struct dgnc_board *brd)
> >  * interrupt context, and there are no locks held.
> >  */
> > brd->channels[i] = kzalloc(sizeof(*brd->channels[i]), 
> > GFP_KERNEL);
> > +   if (!brd->channels[i])
> > +   goto err_free_channels;
> 
> The comments here say that sometimes brd->channels[] are allocated
> earlier.  If that's true then the error handling is not correct.  But
> I don't think it is true...  Could you investigate and delete the
> comments and unnecessary "if (!brd->channels[i])" NULL check.
> 
> regards,
> dan carpenter
> 

I've checked this earlier and now looked over again and I didn't find any other
place where this is allocated. My thought was that deleting that comment and
that check could be too much for one patch. I'll send a v3.

Su pagarba / Regards,
Giedrius

[PATCH v2] staging: dgnc: check if kzalloc fails in dgnc_tty_init()

[Giedrius Statkevičius]

If one of the allocations of memory for storing a channel information struct
fails then free all the successful allocations and return -ENOMEM that gets
propogated to the pci layer.  Also, remove a bogus skipping in the next part of
the initiation if a previous memory allocation failed because we won't execute
that if any of the allocations failed.

Signed-off-by: Giedrius Statkevičius 
---
v2: Only returning -ENOMEM if an allocation failed isn't enough as it was
spotted by Sudip so create a new label that frees all successfully allocated
stuff and only then returns -ENOMEM. Also, remove a unnecessary check in the
next loop.

 drivers/staging/dgnc/dgnc_tty.c | 11 +++
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/dgnc/dgnc_tty.c b/drivers/staging/dgnc/dgnc_tty.c
index ce4187f..60d7e49 100644
--- a/drivers/staging/dgnc/dgnc_tty.c
+++ b/drivers/staging/dgnc/dgnc_tty.c
@@ -316,6 +316,8 @@ int dgnc_tty_init(struct dgnc_board *brd)
 * interrupt context, and there are no locks held.
 */
brd->channels[i] = kzalloc(sizeof(*brd->channels[i]), 
GFP_KERNEL);
+   if (!brd->channels[i])
+   goto err_free_channels;
}
}
 
@@ -324,10 +326,6 @@ int dgnc_tty_init(struct dgnc_board *brd)
 
/* Set up channel variables */
for (i = 0; i < brd->nasync; i++, ch = brd->channels[i]) {
-
-   if (!brd->channels[i])
-   continue;
-
spin_lock_init(>ch_lock);
 
/* Store all our magic numbers */
@@ -375,6 +373,11 @@ int dgnc_tty_init(struct dgnc_board *brd)
}
 
return 0;
+
+err_free_channels:
+   for (i = i - 1; i >= 0; --i)
+   kfree(brd->channels[i]);
+   return -ENOMEM;
 }
 
 
-- 
2.3.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] staging: dgnc: check if kzalloc fails in dgnc_tty_init()

[Giedrius Statkevičius]

On Tue, 7 Apr 2015, Sudip Mukherjee wrote:

> On Tue, Apr 07, 2015 at 01:26:32PM +0300, Giedrius Statkevičius wrote:
> > kzalloc() could fail so add a check and return -ENOMEM if it does that gets
> > propogated to the pci layer
> > 
> > Signed-off-by: Giedrius Statkevičius 
> > ---
> >  drivers/staging/dgnc/dgnc_tty.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/staging/dgnc/dgnc_tty.c 
> > b/drivers/staging/dgnc/dgnc_tty.c
> > index 61d5a8e..23337da 100644
> > --- a/drivers/staging/dgnc/dgnc_tty.c
> > +++ b/drivers/staging/dgnc/dgnc_tty.c
> > @@ -311,6 +311,8 @@ int dgnc_tty_init(struct dgnc_board *brd)
> >  * interrupt context, and there are no locks held.
> >  */
> > brd->channels[i] = kzalloc(sizeof(*brd->channels[i]), 
> > GFP_KERNEL);
> > +   if (!brd->channels[i])
> > +   return -ENOMEM;
> won't this create memory leak ?
> suppose you have brd->nasync = 3 
> and kzalloc fails when i=2, and you return -ENOMEM,
> then what happens to the memory already allocated to brd->channels[0]
> and brd->channels[1] ?
> 
> regards
> sudip
> 
Didn't think about that, sorry. It will cause a memory leak indeed. I'll send a
v2 that creates a label that frees all successful kzalloc() before returning
-ENOMEM.

Su pagarba / Regards,
Giedrius

[PATCH] staging: dgnc: check if kzalloc fails in dgnc_tty_init()

[Giedrius Statkevičius]

kzalloc() could fail so add a check and return -ENOMEM if it does that gets
propogated to the pci layer

Signed-off-by: Giedrius Statkevičius 
---
 drivers/staging/dgnc/dgnc_tty.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/staging/dgnc/dgnc_tty.c b/drivers/staging/dgnc/dgnc_tty.c
index 61d5a8e..23337da 100644
--- a/drivers/staging/dgnc/dgnc_tty.c
+++ b/drivers/staging/dgnc/dgnc_tty.c
@@ -311,6 +311,8 @@ int dgnc_tty_init(struct dgnc_board *brd)
 * interrupt context, and there are no locks held.
 */
brd->channels[i] = kzalloc(sizeof(*brd->channels[i]), 
GFP_KERNEL);
+   if (!brd->channels[i])
+   return -ENOMEM;
}
}
 
-- 
2.3.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] staging: dgnc: check if kzalloc fails in dgnc_tty_init()

[Giedrius Statkevičius]

kzalloc() could fail so add a check and return -ENOMEM if it does that gets
propogated to the pci layer

Signed-off-by: Giedrius Statkevičius giedrius.statkevic...@gmail.com
---
 drivers/staging/dgnc/dgnc_tty.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/staging/dgnc/dgnc_tty.c b/drivers/staging/dgnc/dgnc_tty.c
index 61d5a8e..23337da 100644
--- a/drivers/staging/dgnc/dgnc_tty.c
+++ b/drivers/staging/dgnc/dgnc_tty.c
@@ -311,6 +311,8 @@ int dgnc_tty_init(struct dgnc_board *brd)
 * interrupt context, and there are no locks held.
 */
brd-channels[i] = kzalloc(sizeof(*brd-channels[i]), 
GFP_KERNEL);
+   if (!brd-channels[i])
+   return -ENOMEM;
}
}
 
-- 
2.3.5

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] staging: dgnc: check if kzalloc fails in dgnc_tty_init()

[Giedrius Statkevičius]

On Tue, 7 Apr 2015, Sudip Mukherjee wrote:

 On Tue, Apr 07, 2015 at 01:26:32PM +0300, Giedrius Statkevičius wrote:
  kzalloc() could fail so add a check and return -ENOMEM if it does that gets
  propogated to the pci layer
  
  Signed-off-by: Giedrius Statkevičius giedrius.statkevic...@gmail.com
  ---
   drivers/staging/dgnc/dgnc_tty.c | 2 ++
   1 file changed, 2 insertions(+)
  
  diff --git a/drivers/staging/dgnc/dgnc_tty.c 
  b/drivers/staging/dgnc/dgnc_tty.c
  index 61d5a8e..23337da 100644
  --- a/drivers/staging/dgnc/dgnc_tty.c
  +++ b/drivers/staging/dgnc/dgnc_tty.c
  @@ -311,6 +311,8 @@ int dgnc_tty_init(struct dgnc_board *brd)
   * interrupt context, and there are no locks held.
   */
  brd-channels[i] = kzalloc(sizeof(*brd-channels[i]), 
  GFP_KERNEL);
  +   if (!brd-channels[i])
  +   return -ENOMEM;
 won't this create memory leak ?
 suppose you have brd-nasync = 3 
 and kzalloc fails when i=2, and you return -ENOMEM,
 then what happens to the memory already allocated to brd-channels[0]
 and brd-channels[1] ?
 
 regards
 sudip
 
Didn't think about that, sorry. It will cause a memory leak indeed. I'll send a
v2 that creates a label that frees all successful kzalloc() before returning
-ENOMEM.

Su pagarba / Regards,
Giedrius

[PATCH v2] staging: dgnc: check if kzalloc fails in dgnc_tty_init()

[Giedrius Statkevičius]

If one of the allocations of memory for storing a channel information struct
fails then free all the successful allocations and return -ENOMEM that gets
propogated to the pci layer.  Also, remove a bogus skipping in the next part of
the initiation if a previous memory allocation failed because we won't execute
that if any of the allocations failed.

Signed-off-by: Giedrius Statkevičius giedrius.statkevic...@gmail.com
---
v2: Only returning -ENOMEM if an allocation failed isn't enough as it was
spotted by Sudip so create a new label that frees all successfully allocated
stuff and only then returns -ENOMEM. Also, remove a unnecessary check in the
next loop.

 drivers/staging/dgnc/dgnc_tty.c | 11 +++
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/dgnc/dgnc_tty.c b/drivers/staging/dgnc/dgnc_tty.c
index ce4187f..60d7e49 100644
--- a/drivers/staging/dgnc/dgnc_tty.c
+++ b/drivers/staging/dgnc/dgnc_tty.c
@@ -316,6 +316,8 @@ int dgnc_tty_init(struct dgnc_board *brd)
 * interrupt context, and there are no locks held.
 */
brd-channels[i] = kzalloc(sizeof(*brd-channels[i]), 
GFP_KERNEL);
+   if (!brd-channels[i])
+   goto err_free_channels;
}
}
 
@@ -324,10 +326,6 @@ int dgnc_tty_init(struct dgnc_board *brd)
 
/* Set up channel variables */
for (i = 0; i  brd-nasync; i++, ch = brd-channels[i]) {
-
-   if (!brd-channels[i])
-   continue;
-
spin_lock_init(ch-ch_lock);
 
/* Store all our magic numbers */
@@ -375,6 +373,11 @@ int dgnc_tty_init(struct dgnc_board *brd)
}
 
return 0;
+
+err_free_channels:
+   for (i = i - 1; i = 0; --i)
+   kfree(brd-channels[i]);
+   return -ENOMEM;
 }
 
 
-- 
2.3.5

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v3] staging: dgnc: check if kzalloc fails in dgnc_tty_init()

[Giedrius Statkevičius]

If one of the allocations of memory for storing a channel information struct
fails then free all the successful allocations and return -ENOMEM that gets
propogated to the pci layer.  Also, remove a bogus skipping in the next part of
the initiation if a previous memory allocation failed because we won't execute
that if any of the allocations failed. Next, remove the misleading comment that
allocation could happen elsewhere. Finally, remove all (except in the ioctl
which can try to get information about a board that failed to probe) checks if
-channels[foo] is NULL or not because probe failing if we can't allocate enough
memory means that this scenario isn't possible.

Signed-off-by: Giedrius Statkevičius giedrius.statkevic...@gmail.com
---
v3: Remove the wrong comment at dgnc_tty_init() that says the allocation could
happen somewhere else before this and remove the check if (!brd-channels[i]).
Also, remove all checks where -channels[i] is checked if it's NULL or not
because if probe fails then that means that this scenario isn't possible (except
in the ioctl).

v2: Only returning -ENOMEM if an allocation failed isn't enough as it was
spotted by Sudip so create a new label that frees all successfully allocated
stuff and only then returns -ENOMEM. Also, remove a unnecessary check in the
next loop.

 drivers/staging/dgnc/dgnc_cls.c |  4 +---
 drivers/staging/dgnc/dgnc_neo.c |  2 +-
 drivers/staging/dgnc/dgnc_tty.c | 29 +
 3 files changed, 15 insertions(+), 20 deletions(-)

diff --git a/drivers/staging/dgnc/dgnc_cls.c b/drivers/staging/dgnc/dgnc_cls.c
index e3564d2..a629a78 100644
--- a/drivers/staging/dgnc/dgnc_cls.c
+++ b/drivers/staging/dgnc/dgnc_cls.c
@@ -379,7 +379,7 @@ static inline void cls_parse_isr(struct dgnc_board *brd, 
uint port)
return;
 
ch = brd-channels[port];
-   if (!ch || ch-magic != DGNC_CHANNEL_MAGIC)
+   if (ch-magic != DGNC_CHANNEL_MAGIC)
return;
 
/* Here we try to figure out what caused the interrupt to happen */
@@ -714,8 +714,6 @@ static void cls_tasklet(unsigned long data)
/* Loop on each port */
for (i = 0; i  ports; i++) {
ch = bd-channels[i];
-   if (!ch)
-   continue;
 
/*
 * NOTE: Remember you CANNOT hold any channel
diff --git a/drivers/staging/dgnc/dgnc_neo.c b/drivers/staging/dgnc/dgnc_neo.c
index f5a4d36..1e583c2 100644
--- a/drivers/staging/dgnc/dgnc_neo.c
+++ b/drivers/staging/dgnc/dgnc_neo.c
@@ -395,7 +395,7 @@ static inline void neo_parse_isr(struct dgnc_board *brd, 
uint port)
return;
 
ch = brd-channels[port];
-   if (!ch || ch-magic != DGNC_CHANNEL_MAGIC)
+   if (ch-magic != DGNC_CHANNEL_MAGIC)
return;
 
/* Here we try to figure out what caused the interrupt to happen */
diff --git a/drivers/staging/dgnc/dgnc_tty.c b/drivers/staging/dgnc/dgnc_tty.c
index ce4187f..d1a2c0f 100644
--- a/drivers/staging/dgnc/dgnc_tty.c
+++ b/drivers/staging/dgnc/dgnc_tty.c
@@ -304,19 +304,15 @@ int dgnc_tty_init(struct dgnc_board *brd)
 
brd-nasync = brd-maxports;
 
-   /*
-* Allocate channel memory that might not have been allocated
-* when the driver was first loaded.
-*/
for (i = 0; i  brd-nasync; i++) {
-   if (!brd-channels[i]) {
-
-   /*
-* Okay to malloc with GFP_KERNEL, we are not at
-* interrupt context, and there are no locks held.
-*/
-   brd-channels[i] = kzalloc(sizeof(*brd-channels[i]), 
GFP_KERNEL);
-   }
+   /*
+* Okay to malloc with GFP_KERNEL, we are not at
+* interrupt context, and there are no locks held.
+*/
+   brd-channels[i] = kzalloc(sizeof(*brd-channels[i]),
+  GFP_KERNEL);
+   if (!brd-channels[i])
+   goto err_free_channels;
}
 
ch = brd-channels[0];
@@ -324,10 +320,6 @@ int dgnc_tty_init(struct dgnc_board *brd)
 
/* Set up channel variables */
for (i = 0; i  brd-nasync; i++, ch = brd-channels[i]) {
-
-   if (!brd-channels[i])
-   continue;
-
spin_lock_init(ch-ch_lock);
 
/* Store all our magic numbers */
@@ -375,6 +367,11 @@ int dgnc_tty_init(struct dgnc_board *brd)
}
 
return 0;
+
+err_free_channels:
+   for (i = i - 1; i = 0; --i)
+   kfree(brd-channels[i]);
+   return -ENOMEM;
 }
 
 
-- 
2.3.5

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http

Re: [PATCH v3] staging: dgnc: check if kzalloc fails in dgnc_tty_init()

[Giedrius Statkevičius]

On Tue, 7 Apr 2015, Sudip Mukherjee wrote:

 On Tue, Apr 07, 2015 at 05:11:15PM +0300, Giedrius Statkevičius wrote:
  If one of the allocations of memory for storing a channel information struct
  fails then free all the successful allocations and return -ENOMEM that gets
  propogated to the pci layer.  Also, remove a bogus skipping in the next 
  part of
  the initiation if a previous memory allocation failed because we won't 
  execute
  that if any of the allocations failed. Next, remove the misleading comment 
  that
  allocation could happen elsewhere. Finally, remove all (except in the ioctl
  which can try to get information about a board that failed to probe) checks 
  if
  -channels[foo] is NULL or not because probe failing if we can't allocate 
  enough
  memory means that this scenario isn't possible.
 
 i think now it became too many changes for a single patch..
 
 regards
 sudip

If I split this into two patches then they both would have to be applied and
Greg or someone else could miss that one patch depended on another and leave the
kernel in a partial state so I think it's best to keep it in one. Lets see what
other people have to say.

Su pagarba / Regards,
Giedrius

Re: [PATCH v2] staging: dgnc: check if kzalloc fails in dgnc_tty_init()

[Giedrius Statkevičius]

On Tue, 7 Apr 2015, Dan Carpenter wrote:

 On Tue, Apr 07, 2015 at 03:40:17PM +0300, Giedrius Statkevičius wrote:
  If one of the allocations of memory for storing a channel information struct
  fails then free all the successful allocations and return -ENOMEM that gets
  propogated to the pci layer.  Also, remove a bogus skipping in the next 
  part of
  the initiation if a previous memory allocation failed because we won't 
  execute
  that if any of the allocations failed.
  
  Signed-off-by: Giedrius Statkevičius giedrius.statkevic...@gmail.com
  ---
  v2: Only returning -ENOMEM if an allocation failed isn't enough as it was
  spotted by Sudip so create a new label that frees all successfully allocated
  stuff and only then returns -ENOMEM. Also, remove a unnecessary check in the
  next loop.
  
   drivers/staging/dgnc/dgnc_tty.c | 11 +++
   1 file changed, 7 insertions(+), 4 deletions(-)
  
  diff --git a/drivers/staging/dgnc/dgnc_tty.c 
  b/drivers/staging/dgnc/dgnc_tty.c
  index ce4187f..60d7e49 100644
  --- a/drivers/staging/dgnc/dgnc_tty.c
  +++ b/drivers/staging/dgnc/dgnc_tty.c
  @@ -316,6 +316,8 @@ int dgnc_tty_init(struct dgnc_board *brd)
   * interrupt context, and there are no locks held.
   */
  brd-channels[i] = kzalloc(sizeof(*brd-channels[i]), 
  GFP_KERNEL);
  +   if (!brd-channels[i])
  +   goto err_free_channels;
 
 The comments here say that sometimes brd-channels[] are allocated
 earlier.  If that's true then the error handling is not correct.  But
 I don't think it is true...  Could you investigate and delete the
 comments and unnecessary if (!brd-channels[i]) NULL check.
 
 regards,
 dan carpenter
 

I've checked this earlier and now looked over again and I didn't find any other
place where this is allocated. My thought was that deleting that comment and
that check could be too much for one patch. I'll send a v3.

Su pagarba / Regards,
Giedrius

Re: [PATCH v3] staging: dgnc: check if kzalloc fails in dgnc_tty_init()

[Giedrius Statkevičius]

On Tue, 7 Apr 2015, Dan Carpenter wrote:

 You will need to update the subject to reflect the new patch.
 
 The original code did check for kzalloc() failure but it had lots of
 checks scattered around instead nicely at the point where the memory
 was allocated.
 

There are a lot missing too. For example in dgnc_sysfs.c there are no checks on
any _show() methods if -channels[i] is NULL or not. And in some other places.

 The old code and the new code are both buggy though and will crash in
 dgnc_tty_uninit().  dgnc_found_board() does One Err style error
 handling so it's obviously buggy like the underside of a rock.
 https://plus.google.com/106378716002406849458/posts/dnanfhQ4mHQ
 
 It's becoming a difficult thing to fix this because every time we look
 there are more things which don't make sense.
 
 I believe that if you do:
 
  +err_free_channels:
  +   for (i = i - 1; i = 0; --i) {
  +   kfree(brd-channels[i]);
   brd-channels[i] = NULL;
   }
  +   return -ENOMEM;
   }
 
 And add some NULL checks in dgnc_tty_uninit() to see if -channels[i] is
 NULL before doing:
 
   dgnc_remove_tty_sysfs(brd-channels[i]-ch_tun.un_sysfs);
 
 and
   dgnc_remove_tty_sysfs(brd-channels[i]-ch_pun.un_sysfs);
 
 Then it will fix the bug.
 

Missed this. Yep, I agree. 

 Do these in a separate patch.  I'm looking for ways we can make this
 patch minimal.  Deleting the comments and the NULL check in
 dgnc_tty_init() is essential for the patch because otherwise the cleanup
 doesn't make sense.

Well, the point of the patch is to put alloc and checks in one place to make the
code less error and bug prone and fix some of bugs where -channels[i] isn't
checked. Ok, I'll send a v5 that's split into two patches.

 
 regards,
 dan carpenter
 
 

Su pagarba / Regards,
Giedrius
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v4 2/3] staging: dgnc: don't forget to check if -channels[i] is NULL in dgnc_tty_uninit()

[Giedrius Statkevičius]

Add a check if -channels[i] is NULL because a NULL pointer may be dereferenced
in case one of the allocations failed

Reported-by: Dan Carpenter dan.carpen...@oracle.com
Signed-off-by: Giedrius Statkevičius giedrius.statkevic...@gmail.com
---
v4: new patch that fixes a bug reported by Dan Carpenter

 drivers/staging/dgnc/dgnc_tty.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/dgnc/dgnc_tty.c b/drivers/staging/dgnc/dgnc_tty.c
index 0e48f95..785eb6c 100644
--- a/drivers/staging/dgnc/dgnc_tty.c
+++ b/drivers/staging/dgnc/dgnc_tty.c
@@ -403,7 +403,9 @@ void dgnc_tty_uninit(struct dgnc_board *brd)
dgnc_BoardsByMajor[brd-SerialDriver.major] = NULL;
brd-dgnc_Serial_Major = 0;
for (i = 0; i  brd-nasync; i++) {
-   
dgnc_remove_tty_sysfs(brd-channels[i]-ch_tun.un_sysfs);
+   if (brd-channels[i])
+   dgnc_remove_tty_sysfs(brd-channels[i]-
+ ch_tun.un_sysfs);
tty_unregister_device(brd-SerialDriver, i);
}
tty_unregister_driver(brd-SerialDriver);
@@ -414,7 +416,9 @@ void dgnc_tty_uninit(struct dgnc_board *brd)
dgnc_BoardsByMajor[brd-PrintDriver.major] = NULL;
brd-dgnc_TransparentPrint_Major = 0;
for (i = 0; i  brd-nasync; i++) {
-   
dgnc_remove_tty_sysfs(brd-channels[i]-ch_pun.un_sysfs);
+   if (brd-channels[i])
+   dgnc_remove_tty_sysfs(brd-channels[i]-
+ ch_pun.un_sysfs);
tty_unregister_device(brd-PrintDriver, i);
}
tty_unregister_driver(brd-PrintDriver);
-- 
2.3.5

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v4 1/3] staging: dgnc: clean up allocation of -channels[i]

[Giedrius Statkevičius]

Check if kzalloc fails in dgnc_tty_init() and if it does then free all
previously allocated -channels[i] and set them to NULL. This makes the code
less error/bug prone because instead of needing programmers attention to add
checks everywhere we do that in one place. Also, remove a bogus comment and
check in the same loop because -channels[i] isn't allocated anywhere else.
Finally, remove a unnecessary check if -channels[i] is NULL in the next loop
because it can't be.

Signed-off-by: Giedrius Statkevičius giedrius.statkevic...@gmail.com
---
v4: Make this patch only for dgnc_tty.c and only for this thing. Split into 3
patches (1 patch fixes a bug reported by Dan Carpenter)

v3: Remove the wrong comment at dgnc_tty_init() that says the allocation could
happen somewhere else before this and remove the check if (!brd-channels[i]).
Also, remove all checks where -channels[i] is checked if it's NULL or not
because if probe fails then that means that this scenario isn't possible (except
in the ioctl).

v2: Only returning -ENOMEM if an allocation failed isn't enough as it was
spotted by Sudip so create a new label that frees all successfully allocated
stuff and only then returns -ENOMEM. Also, remove a unnecessary check in the
next loop.

 drivers/staging/dgnc/dgnc_tty.c | 31 +++
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/drivers/staging/dgnc/dgnc_tty.c b/drivers/staging/dgnc/dgnc_tty.c
index ce4187f..0e48f95 100644
--- a/drivers/staging/dgnc/dgnc_tty.c
+++ b/drivers/staging/dgnc/dgnc_tty.c
@@ -304,19 +304,15 @@ int dgnc_tty_init(struct dgnc_board *brd)
 
brd-nasync = brd-maxports;
 
-   /*
-* Allocate channel memory that might not have been allocated
-* when the driver was first loaded.
-*/
for (i = 0; i  brd-nasync; i++) {
-   if (!brd-channels[i]) {
-
-   /*
-* Okay to malloc with GFP_KERNEL, we are not at
-* interrupt context, and there are no locks held.
-*/
-   brd-channels[i] = kzalloc(sizeof(*brd-channels[i]), 
GFP_KERNEL);
-   }
+   /*
+* Okay to malloc with GFP_KERNEL, we are not at
+* interrupt context, and there are no locks held.
+*/
+   brd-channels[i] = kzalloc(sizeof(*brd-channels[i]),
+  GFP_KERNEL);
+   if (!brd-channels[i])
+   goto err_free_channels;
}
 
ch = brd-channels[0];
@@ -324,10 +320,6 @@ int dgnc_tty_init(struct dgnc_board *brd)
 
/* Set up channel variables */
for (i = 0; i  brd-nasync; i++, ch = brd-channels[i]) {
-
-   if (!brd-channels[i])
-   continue;
-
spin_lock_init(ch-ch_lock);
 
/* Store all our magic numbers */
@@ -375,6 +367,13 @@ int dgnc_tty_init(struct dgnc_board *brd)
}
 
return 0;
+
+err_free_channels:
+   for (i = i - 1; i = 0; --i) {
+   kfree(brd-channels[i]);
+   brd-channels[i] = NULL;
+   }
+   return -ENOMEM;
 }
 
 
-- 
2.3.5

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v4 3/3] staging: dgnc: remove redundant !ch checks

[Giedrius Statkevičius]

Remove checks that are redundant since we don't have boards with partially
initialized -channels[i].

Signed-off-by: Giedrius Statkevičius giedrius.statkevic...@gmail.com
---
v4: splitted this from the one patch.

 drivers/staging/dgnc/dgnc_cls.c | 4 ++--
 drivers/staging/dgnc/dgnc_neo.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/dgnc/dgnc_cls.c b/drivers/staging/dgnc/dgnc_cls.c
index e3564d2..82e8680 100644
--- a/drivers/staging/dgnc/dgnc_cls.c
+++ b/drivers/staging/dgnc/dgnc_cls.c
@@ -379,7 +379,7 @@ static inline void cls_parse_isr(struct dgnc_board *brd, 
uint port)
return;
 
ch = brd-channels[port];
-   if (!ch || ch-magic != DGNC_CHANNEL_MAGIC)
+   if (ch-magic != DGNC_CHANNEL_MAGIC)
return;
 
/* Here we try to figure out what caused the interrupt to happen */
@@ -691,7 +691,7 @@ static void cls_tasklet(unsigned long data)
int state = 0;
int ports = 0;
 
-   if (!bd || bd-magic != DGNC_BOARD_MAGIC)
+   if (bd-magic != DGNC_BOARD_MAGIC)
return;
 
/* Cache a couple board values */
diff --git a/drivers/staging/dgnc/dgnc_neo.c b/drivers/staging/dgnc/dgnc_neo.c
index f5a4d36..1e583c2 100644
--- a/drivers/staging/dgnc/dgnc_neo.c
+++ b/drivers/staging/dgnc/dgnc_neo.c
@@ -395,7 +395,7 @@ static inline void neo_parse_isr(struct dgnc_board *brd, 
uint port)
return;
 
ch = brd-channels[port];
-   if (!ch || ch-magic != DGNC_CHANNEL_MAGIC)
+   if (ch-magic != DGNC_CHANNEL_MAGIC)
return;
 
/* Here we try to figure out what caused the interrupt to happen */
-- 
2.3.5

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/2] staging: dgnc: remove dead code in dgnc_tty_write()

[Giedrius Statkevičius]

Forgot to put "v2" in the title. Should I resend? Sorry for the confusion. Still
making some patch submitting mistakes from time to time. :(

On Mon, 6 Apr 2015, Giedrius Statkevicius wrote:

> Remove the dead code protected by in_user in dgnc_tty_write() because it is 
> set
> to 0 and never changed to 1 thus the code in ifs never gets executed.
> 
> Signed-off-by: Giedrius Statkevičius 
> ---
> v2: Just remove the dead code protected by in_user and join the first and 
> third
> patches.
> 
>  drivers/staging/dgnc/dgnc_tty.c | 45 
> +
>  1 file changed, 1 insertion(+), 44 deletions(-)
> 
> diff --git a/drivers/staging/dgnc/dgnc_tty.c b/drivers/staging/dgnc/dgnc_tty.c
> index ce4187f..cf2eb21 100644
> --- a/drivers/staging/dgnc/dgnc_tty.c
> +++ b/drivers/staging/dgnc/dgnc_tty.c
> @@ -42,16 +42,11 @@
>  #include "dgnc_sysfs.h"
>  #include "dgnc_utils.h"
>  
> -#define init_MUTEX(sem)   sema_init(sem, 1)
> -#define DECLARE_MUTEX(name) \
> - struct semaphore name = __SEMAPHORE_INITIALIZER(name, 1)
> -
>  /*
>   * internal variables
>   */
>  static struct dgnc_board *dgnc_BoardsByMajor[256];
>  static unsigned char *dgnc_TmpWriteBuf;
> -static DECLARE_MUTEX(dgnc_TmpWriteSem);
>  
>  /*
>   * Default transparent print information.
> @@ -1705,7 +1700,6 @@ static int dgnc_tty_write(struct tty_struct *tty,
>   ushort tail;
>   ushort tmask;
>   uint remain;
> - int from_user = 0;
>  
>   if (tty == NULL || dgnc_TmpWriteBuf == NULL)
>   return 0;
> @@ -1785,38 +1779,6 @@ static int dgnc_tty_write(struct tty_struct *tty,
>   if (count <= 0)
>   goto exit_retry;
>  
> - if (from_user) {
> -
> - count = min(count, WRITEBUFLEN);
> -
> - spin_unlock_irqrestore(>ch_lock, flags);
> -
> - /*
> -  * If data is coming from user space, copy it into a temporary
> -  * buffer so we don't get swapped out while doing the copy to
> -  * the board.
> -  */
> - /* we're allowed to block if it's from_user */
> - if (down_interruptible(_TmpWriteSem))
> - return -EINTR;
> -
> - /*
> -  * copy_from_user() returns the number
> -  * of bytes that could *NOT* be copied.
> -  */
> - count -= copy_from_user(dgnc_TmpWriteBuf, (const unsigned char 
> __user *) buf, count);
> -
> - if (!count) {
> - up(_TmpWriteSem);
> - return -EFAULT;
> - }
> -
> - spin_lock_irqsave(>ch_lock, flags);
> -
> - buf = dgnc_TmpWriteBuf;
> -
> - }
> -
>   n = count;
>  
>   /*
> @@ -1853,12 +1815,7 @@ static int dgnc_tty_write(struct tty_struct *tty,
>   ch->ch_cpstime += (HZ * count) / ch->ch_digi.digi_maxcps;
>   }
>  
> - if (from_user) {
> - spin_unlock_irqrestore(>ch_lock, flags);
> - up(_TmpWriteSem);
> - } else {
> - spin_unlock_irqrestore(>ch_lock, flags);
> - }
> + spin_unlock_irqrestore(>ch_lock, flags);
>  
>   if (count) {
>   /*
> -- 
> 2.3.5
> 
> 

Su pagarba / Regards,
Giedrius

Re: [PATCH 1/2] staging: dgnc: remove dead code in dgnc_tty_write()

[Giedrius Statkevičius]

Forgot to put v2 in the title. Should I resend? Sorry for the confusion. Still
making some patch submitting mistakes from time to time. :(

On Mon, 6 Apr 2015, Giedrius Statkevicius wrote:

 Remove the dead code protected by in_user in dgnc_tty_write() because it is 
 set
 to 0 and never changed to 1 thus the code in ifs never gets executed.
 
 Signed-off-by: Giedrius Statkevičius giedrius.statkevic...@gmail.com
 ---
 v2: Just remove the dead code protected by in_user and join the first and 
 third
 patches.
 
  drivers/staging/dgnc/dgnc_tty.c | 45 
 +
  1 file changed, 1 insertion(+), 44 deletions(-)
 
 diff --git a/drivers/staging/dgnc/dgnc_tty.c b/drivers/staging/dgnc/dgnc_tty.c
 index ce4187f..cf2eb21 100644
 --- a/drivers/staging/dgnc/dgnc_tty.c
 +++ b/drivers/staging/dgnc/dgnc_tty.c
 @@ -42,16 +42,11 @@
  #include dgnc_sysfs.h
  #include dgnc_utils.h
  
 -#define init_MUTEX(sem)   sema_init(sem, 1)
 -#define DECLARE_MUTEX(name) \
 - struct semaphore name = __SEMAPHORE_INITIALIZER(name, 1)
 -
  /*
   * internal variables
   */
  static struct dgnc_board *dgnc_BoardsByMajor[256];
  static unsigned char *dgnc_TmpWriteBuf;
 -static DECLARE_MUTEX(dgnc_TmpWriteSem);
  
  /*
   * Default transparent print information.
 @@ -1705,7 +1700,6 @@ static int dgnc_tty_write(struct tty_struct *tty,
   ushort tail;
   ushort tmask;
   uint remain;
 - int from_user = 0;
  
   if (tty == NULL || dgnc_TmpWriteBuf == NULL)
   return 0;
 @@ -1785,38 +1779,6 @@ static int dgnc_tty_write(struct tty_struct *tty,
   if (count = 0)
   goto exit_retry;
  
 - if (from_user) {
 -
 - count = min(count, WRITEBUFLEN);
 -
 - spin_unlock_irqrestore(ch-ch_lock, flags);
 -
 - /*
 -  * If data is coming from user space, copy it into a temporary
 -  * buffer so we don't get swapped out while doing the copy to
 -  * the board.
 -  */
 - /* we're allowed to block if it's from_user */
 - if (down_interruptible(dgnc_TmpWriteSem))
 - return -EINTR;
 -
 - /*
 -  * copy_from_user() returns the number
 -  * of bytes that could *NOT* be copied.
 -  */
 - count -= copy_from_user(dgnc_TmpWriteBuf, (const unsigned char 
 __user *) buf, count);
 -
 - if (!count) {
 - up(dgnc_TmpWriteSem);
 - return -EFAULT;
 - }
 -
 - spin_lock_irqsave(ch-ch_lock, flags);
 -
 - buf = dgnc_TmpWriteBuf;
 -
 - }
 -
   n = count;
  
   /*
 @@ -1853,12 +1815,7 @@ static int dgnc_tty_write(struct tty_struct *tty,
   ch-ch_cpstime += (HZ * count) / ch-ch_digi.digi_maxcps;
   }
  
 - if (from_user) {
 - spin_unlock_irqrestore(ch-ch_lock, flags);
 - up(dgnc_TmpWriteSem);
 - } else {
 - spin_unlock_irqrestore(ch-ch_lock, flags);
 - }
 + spin_unlock_irqrestore(ch-ch_lock, flags);
  
   if (count) {
   /*
 -- 
 2.3.5
 
 

Su pagarba / Regards,
Giedrius

Re: [PATCH] staging: sm7xxfb: Fix sparse warning

[Giedrius Statkevičius]

On Sat, 4 Apr 2015, Nickolaus Woodruff wrote:

> This patch fixes the following sparse warning in sm7xx.h:
> 
> drivers/staging/sm7xxfb/sm7xx.h:122:17: warning: symbol 'vgamode'
> was not declared. Should it be static?
> 
> Signed-off-by: Nickolaus Woodruff 
> ---
>  drivers/staging/sm7xxfb/sm7xx.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/sm7xxfb/sm7xx.h b/drivers/staging/sm7xxfb/sm7xx.h
> index 7cc1896..c5d6253 100644
> --- a/drivers/staging/sm7xxfb/sm7xx.h
> +++ b/drivers/staging/sm7xxfb/sm7xx.h
> @@ -119,7 +119,7 @@ struct ModeInit {
>  /**
>SM712 Mode table.
>   **/
> -struct ModeInit vgamode[] = {
> +static struct ModeInit vgamode[] = {

Someone already fixed this:
https://git.kernel.org/cgit/linux/kernel/git/gregkh/staging.git/tree/drivers/staging/sm7xxfb/sm7xx.h?h=staging-testing#n122
Please work against staging-testing branch of the staging tree.

Su pagarba / Regards,
Giedrius
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] staging: sm7xxfb: Fix sparse warning

[Giedrius Statkevičius]

On Sat, 4 Apr 2015, Nickolaus Woodruff wrote:

 This patch fixes the following sparse warning in sm7xx.h:
 
 drivers/staging/sm7xxfb/sm7xx.h:122:17: warning: symbol 'vgamode'
 was not declared. Should it be static?
 
 Signed-off-by: Nickolaus Woodruff nickolauswoodr...@gmail.com
 ---
  drivers/staging/sm7xxfb/sm7xx.h | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/drivers/staging/sm7xxfb/sm7xx.h b/drivers/staging/sm7xxfb/sm7xx.h
 index 7cc1896..c5d6253 100644
 --- a/drivers/staging/sm7xxfb/sm7xx.h
 +++ b/drivers/staging/sm7xxfb/sm7xx.h
 @@ -119,7 +119,7 @@ struct ModeInit {
  /**
SM712 Mode table.
   **/
 -struct ModeInit vgamode[] = {
 +static struct ModeInit vgamode[] = {

Someone already fixed this:
https://git.kernel.org/cgit/linux/kernel/git/gregkh/staging.git/tree/drivers/staging/sm7xxfb/sm7xx.h?h=staging-testing#n122
Please work against staging-testing branch of the staging tree.

Su pagarba / Regards,
Giedrius
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] staging: fbtft: 80 characters per line in fb_ili9163.c

[Giedrius Statkevičius]

On Sat, 4 Apr 2015, Andreas Theodosiou wrote:

> This is a patch to the fb_ili9163.c file that inserts a line break in
> line #92 to make the line fit into the 80 character limit.
> 
> Signed-off-by: Andreas Theodosiou 
> ---
>  drivers/staging/fbtft/fb_ili9163.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/fbtft/fb_ili9163.c 
> b/drivers/staging/fbtft/fb_ili9163.c
> index ed92a64..fc7568b 100644
> --- a/drivers/staging/fbtft/fb_ili9163.c
> +++ b/drivers/staging/fbtft/fb_ili9163.c
> @@ -89,7 +89,8 @@
>  
>  /*
>  This display:
> -http://www.ebay.com/itm/Replace-Nokia-5110-LCD-1-44-Red-Serial-128X128-SPI-Color-TFT-LCD-Display-Module-/271422122271
> +http://www.ebay.com/itm/Replace-Nokia-5110-LCD-1-44-Red-Serial-128X128-SPI-
> +Color-TFT-LCD-Display-Module-/271422122271

With this change the user won't be able to just simply click on the url and have
it open for him. Thus I don't think this is a good change. Sometimes in the
kernel people leave lines over 80 characters to make the code more grep-able,
sometimes it's just more clear when it's over 80 characters or, probably, in
cases like this.

Su pagarba / Regards,
Giedrius
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] staging: fbtft: 80 characters per line in fb_ili9163.c

[Giedrius Statkevičius]

On Sat, 4 Apr 2015, Andreas Theodosiou wrote:

 This is a patch to the fb_ili9163.c file that inserts a line break in
 line #92 to make the line fit into the 80 character limit.
 
 Signed-off-by: Andreas Theodosiou andreas...@gmail.com
 ---
  drivers/staging/fbtft/fb_ili9163.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)
 
 diff --git a/drivers/staging/fbtft/fb_ili9163.c 
 b/drivers/staging/fbtft/fb_ili9163.c
 index ed92a64..fc7568b 100644
 --- a/drivers/staging/fbtft/fb_ili9163.c
 +++ b/drivers/staging/fbtft/fb_ili9163.c
 @@ -89,7 +89,8 @@
  
  /*
  This display:
 -http://www.ebay.com/itm/Replace-Nokia-5110-LCD-1-44-Red-Serial-128X128-SPI-Color-TFT-LCD-Display-Module-/271422122271
 +http://www.ebay.com/itm/Replace-Nokia-5110-LCD-1-44-Red-Serial-128X128-SPI-
 +Color-TFT-LCD-Display-Module-/271422122271

With this change the user won't be able to just simply click on the url and have
it open for him. Thus I don't think this is a good change. Sometimes in the
kernel people leave lines over 80 characters to make the code more grep-able,
sometimes it's just more clear when it's over 80 characters or, probably, in
cases like this.

Su pagarba / Regards,
Giedrius
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] sm750fb: Fix Comment and if else braces

[Giedrius Statkevičius]

Hi Amitoj Kaur Chawla,

On 2015.03.19 20:05, Amitoj Kaur Chawla wrote:
> The edits have been made to add a space before the comment and the errors in
> braces in the if-else statements.
> Previously in commit 2d34f53f1065878cd881ac61a183f8e836583d51, removed
> the C99 comments and errors in if-else indentation and braces.

The previous patch isn't in the git repo yet so it doesn't have a commit
id thus this one is wrong. You should've just made a v2 and made that
change. I recommend you to look over this:
http://kernelnewbies.org/FirstKernelPatch Just remove the HEAD commit in
your local git repo, add that space and then when using `git
format-patch` use the "--subject-prefix" option to add that it's a
second version. Then after "---" you can say what was changed in the
second version. For example (I'm sure you can think of a better one!):

Signed-off-by: ...
---
v2: added a space after a statement in a line where a coding style error
was fixed.

drivers/...

>  
> Signed-off-by: Amitoj Kaur Chawla 
> ---
>  drivers/staging/sm750fb/ddk750_chip.c | 11 ---
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/staging/sm750fb/ddk750_chip.c 
> b/drivers/staging/sm750fb/ddk750_chip.c
> index 33fa456..bdf6a73 100644
> --- a/drivers/staging/sm750fb/ddk750_chip.c
> +++ b/drivers/staging/sm750fb/ddk750_chip.c
> @@ -17,20 +17,17 @@ logical_chip_type_t getChipType(void)
>   char physicalRev;
>   logical_chip_type_t chip;
>  
> - physicalID = devId750;/* either 0x718 or 0x750 */
> + physicalID = devId750; /* either 0x718 or 0x750 */
>   physicalRev = revId750;
>  
>   if (physicalID == 0x718) {
>   chip = SM718;
> - }
> - else if (physicalID == 0x750) {
> + } else if (physicalID == 0x750) {
>   chip = SM750;
>   /* SM750 and SM750LE are different in their revision ID only. */
> - if (physicalRev == SM750LE_REVISION_ID) {
> + if (physicalRev == SM750LE_REVISION_ID)
>   chip = SM750LE;
> - }
> - }
> - else {
> + } else {
>   chip = SM_UNKNOWN;
>   }
>  
> 


-- 
Thanks,
Giedrius
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] sm750fb: Fix C99 Comments and if else braces

[Giedrius Statkevičius]

Hi Amitoj Kaur Chawla,

On 2015.03.19 19:39, Amitoj Kaur Chawla wrote:
> The edits have been made to remove C99 Comments and properly indent
> the if-else statements in the file while taking care of the braces according 
> to
> Linux coding style.
> 
> Signed-off-by: Amitoj Kaur Chawla 
> ---
>  drivers/staging/sm750fb/ddk750_chip.c | 25 +++--
>  1 file changed, 11 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/staging/sm750fb/ddk750_chip.c 
> b/drivers/staging/sm750fb/ddk750_chip.c
> index 33add64..33fa456 100644
> --- a/drivers/staging/sm750fb/ddk750_chip.c
> +++ b/drivers/staging/sm750fb/ddk750_chip.c
> @@ -17,25 +17,22 @@ logical_chip_type_t getChipType(void)
>   char physicalRev;
>   logical_chip_type_t chip;
>  
> - physicalID = devId750;//either 0x718 or 0x750
> + physicalID = devId750;/* either 0x718 or 0x750 */

Add a space here after ;?

>   physicalRev = revId750;
>  
> -if (physicalID == 0x718)
> -{
> -chip = SM718;
> -}
> -else if (physicalID == 0x750)
> -{
> -chip = SM750;
> + if (physicalID == 0x718) {
> + chip = SM718;
> + }
> + else if (physicalID == 0x750) {
> + chip = SM750;
>   /* SM750 and SM750LE are different in their revision ID only. */
> - if (physicalRev == SM750LE_REVISION_ID){
> + if (physicalRev == SM750LE_REVISION_ID) {
>   chip = SM750LE;
>   }
> -}
> -else
> -{
> -chip = SM_UNKNOWN;
> -}
> + }
> + else {
> + chip = SM_UNKNOWN;
> + }
>  
>   return chip;
>  }
> 


-- 
Thanks,
Giedrius
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] sm750fb: Fix C99 Comments and if else braces

[Giedrius Statkevičius]

Hi Amitoj Kaur Chawla,

On 2015.03.19 19:39, Amitoj Kaur Chawla wrote:
 The edits have been made to remove C99 Comments and properly indent
 the if-else statements in the file while taking care of the braces according 
 to
 Linux coding style.
 
 Signed-off-by: Amitoj Kaur Chawla amitoj1...@gmail.com
 ---
  drivers/staging/sm750fb/ddk750_chip.c | 25 +++--
  1 file changed, 11 insertions(+), 14 deletions(-)
 
 diff --git a/drivers/staging/sm750fb/ddk750_chip.c 
 b/drivers/staging/sm750fb/ddk750_chip.c
 index 33add64..33fa456 100644
 --- a/drivers/staging/sm750fb/ddk750_chip.c
 +++ b/drivers/staging/sm750fb/ddk750_chip.c
 @@ -17,25 +17,22 @@ logical_chip_type_t getChipType(void)
   char physicalRev;
   logical_chip_type_t chip;
  
 - physicalID = devId750;//either 0x718 or 0x750
 + physicalID = devId750;/* either 0x718 or 0x750 */

Add a space here after ;?

   physicalRev = revId750;
  
 -if (physicalID == 0x718)
 -{
 -chip = SM718;
 -}
 -else if (physicalID == 0x750)
 -{
 -chip = SM750;
 + if (physicalID == 0x718) {
 + chip = SM718;
 + }
 + else if (physicalID == 0x750) {
 + chip = SM750;
   /* SM750 and SM750LE are different in their revision ID only. */
 - if (physicalRev == SM750LE_REVISION_ID){
 + if (physicalRev == SM750LE_REVISION_ID) {
   chip = SM750LE;
   }
 -}
 -else
 -{
 -chip = SM_UNKNOWN;
 -}
 + }
 + else {
 + chip = SM_UNKNOWN;
 + }
  
   return chip;
  }
 


-- 
Thanks,
Giedrius
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] sm750fb: Fix Comment and if else braces

[Giedrius Statkevičius]

Hi Amitoj Kaur Chawla,

On 2015.03.19 20:05, Amitoj Kaur Chawla wrote:
 The edits have been made to add a space before the comment and the errors in
 braces in the if-else statements.
 Previously in commit 2d34f53f1065878cd881ac61a183f8e836583d51, removed
 the C99 comments and errors in if-else indentation and braces.

The previous patch isn't in the git repo yet so it doesn't have a commit
id thus this one is wrong. You should've just made a v2 and made that
change. I recommend you to look over this:
http://kernelnewbies.org/FirstKernelPatch Just remove the HEAD commit in
your local git repo, add that space and then when using `git
format-patch` use the --subject-prefix option to add that it's a
second version. Then after --- you can say what was changed in the
second version. For example (I'm sure you can think of a better one!):

Signed-off-by: ...
---
v2: added a space after a statement in a line where a coding style error
was fixed.

drivers/...

  
 Signed-off-by: Amitoj Kaur Chawla amitoj1...@gmail.com
 ---
  drivers/staging/sm750fb/ddk750_chip.c | 11 ---
  1 file changed, 4 insertions(+), 7 deletions(-)
 
 diff --git a/drivers/staging/sm750fb/ddk750_chip.c 
 b/drivers/staging/sm750fb/ddk750_chip.c
 index 33fa456..bdf6a73 100644
 --- a/drivers/staging/sm750fb/ddk750_chip.c
 +++ b/drivers/staging/sm750fb/ddk750_chip.c
 @@ -17,20 +17,17 @@ logical_chip_type_t getChipType(void)
   char physicalRev;
   logical_chip_type_t chip;
  
 - physicalID = devId750;/* either 0x718 or 0x750 */
 + physicalID = devId750; /* either 0x718 or 0x750 */
   physicalRev = revId750;
  
   if (physicalID == 0x718) {
   chip = SM718;
 - }
 - else if (physicalID == 0x750) {
 + } else if (physicalID == 0x750) {
   chip = SM750;
   /* SM750 and SM750LE are different in their revision ID only. */
 - if (physicalRev == SM750LE_REVISION_ID) {
 + if (physicalRev == SM750LE_REVISION_ID)
   chip = SM750LE;
 - }
 - }
 - else {
 + } else {
   chip = SM_UNKNOWN;
   }
  
 


-- 
Thanks,
Giedrius
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 1/7] dgnc: remove unused dgnc_ioctl_name() command

[Giedrius Statkevičius]

Greg, you probably missed these :) Could you look at these when you have
some time?

On 2015.03.13 15:56, Giedrius Statkevičius wrote:
> dgnc_ioctl_name() is never used anywhere so remove it
> 
> Signed-off-by: Giedrius Statkevičius 
> ---
> v2: Forgot to include this patch in the first version. Without this some
> of the other patches in this set will fail.
> 
>  drivers/staging/dgnc/dgnc_utils.c | 52 
> ---
>  drivers/staging/dgnc/dgnc_utils.h |  1 -
>  2 files changed, 53 deletions(-)
> 
> diff --git a/drivers/staging/dgnc/dgnc_utils.c 
> b/drivers/staging/dgnc/dgnc_utils.c
> index 80b5133..f76de82 100644
> --- a/drivers/staging/dgnc/dgnc_utils.c
> +++ b/drivers/staging/dgnc/dgnc_utils.c
> @@ -16,55 +16,3 @@ int dgnc_ms_sleep(ulong ms)
>   schedule_timeout((ms * HZ) / 1000);
>   return signal_pending(current);
>  }
> -
> -/*
> - *  dgnc_ioctl_name() : Returns a text version of each ioctl value.
> - */
> -char *dgnc_ioctl_name(int cmd)
> -{
> - switch (cmd) {
> -
> - case TCGETA:return "TCGETA";
> - case TCGETS:return "TCGETS";
> - case TCSETA:return "TCSETA";
> - case TCSETS:return "TCSETS";
> - case TCSETAW:   return "TCSETAW";
> - case TCSETSW:   return "TCSETSW";
> - case TCSETAF:   return "TCSETAF";
> - case TCSETSF:   return "TCSETSF";
> - case TCSBRK:return "TCSBRK";
> - case TCXONC:return "TCXONC";
> - case TCFLSH:return "TCFLSH";
> - case TIOCGSID:  return "TIOCGSID";
> -
> - case TIOCGETD:  return "TIOCGETD";
> - case TIOCSETD:  return "TIOCSETD";
> - case TIOCGWINSZ:return "TIOCGWINSZ";
> - case TIOCSWINSZ:return "TIOCSWINSZ";
> -
> - case TIOCMGET:  return "TIOCMGET";
> - case TIOCMSET:  return "TIOCMSET";
> - case TIOCMBIS:  return "TIOCMBIS";
> - case TIOCMBIC:  return "TIOCMBIC";
> -
> - /* from digi.h */
> - case DIGI_SETA: return "DIGI_SETA";
> - case DIGI_SETAW:return "DIGI_SETAW";
> - case DIGI_SETAF:return "DIGI_SETAF";
> - case DIGI_SETFLOW:  return "DIGI_SETFLOW";
> - case DIGI_SETAFLOW: return "DIGI_SETAFLOW";
> - case DIGI_GETFLOW:  return "DIGI_GETFLOW";
> - case DIGI_GETAFLOW: return "DIGI_GETAFLOW";
> - case DIGI_GETA: return "DIGI_GETA";
> - case DIGI_GEDELAY:  return "DIGI_GEDELAY";
> - case DIGI_SEDELAY:  return "DIGI_SEDELAY";
> - case DIGI_GETCUSTOMBAUD: return "DIGI_GETCUSTOMBAUD";
> - case DIGI_SETCUSTOMBAUD: return "DIGI_SETCUSTOMBAUD";
> - case TIOCMODG:  return "TIOCMODG";
> - case TIOCMODS:  return "TIOCMODS";
> - case TIOCSDTR:  return "TIOCSDTR";
> - case TIOCCDTR:  return "TIOCCDTR";
> -
> - default:return "unknown";
> - }
> -}
> diff --git a/drivers/staging/dgnc/dgnc_utils.h 
> b/drivers/staging/dgnc/dgnc_utils.h
> index cebf601..1164c3a 100644
> --- a/drivers/staging/dgnc/dgnc_utils.h
> +++ b/drivers/staging/dgnc/dgnc_utils.h
> @@ -2,6 +2,5 @@
>  #define __DGNC_UTILS_H
>  
>  int dgnc_ms_sleep(ulong ms);
> -char *dgnc_ioctl_name(int cmd);
>  
>  #endif
> 


-- 
Thanks,
Giedrius
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 1/7] dgnc: remove unused dgnc_ioctl_name() command

[Giedrius Statkevičius]

Greg, you probably missed these :) Could you look at these when you have
some time?

On 2015.03.13 15:56, Giedrius Statkevičius wrote:
 dgnc_ioctl_name() is never used anywhere so remove it
 
 Signed-off-by: Giedrius Statkevičius giedrius.statkevic...@gmail.com
 ---
 v2: Forgot to include this patch in the first version. Without this some
 of the other patches in this set will fail.
 
  drivers/staging/dgnc/dgnc_utils.c | 52 
 ---
  drivers/staging/dgnc/dgnc_utils.h |  1 -
  2 files changed, 53 deletions(-)
 
 diff --git a/drivers/staging/dgnc/dgnc_utils.c 
 b/drivers/staging/dgnc/dgnc_utils.c
 index 80b5133..f76de82 100644
 --- a/drivers/staging/dgnc/dgnc_utils.c
 +++ b/drivers/staging/dgnc/dgnc_utils.c
 @@ -16,55 +16,3 @@ int dgnc_ms_sleep(ulong ms)
   schedule_timeout((ms * HZ) / 1000);
   return signal_pending(current);
  }
 -
 -/*
 - *  dgnc_ioctl_name() : Returns a text version of each ioctl value.
 - */
 -char *dgnc_ioctl_name(int cmd)
 -{
 - switch (cmd) {
 -
 - case TCGETA:return TCGETA;
 - case TCGETS:return TCGETS;
 - case TCSETA:return TCSETA;
 - case TCSETS:return TCSETS;
 - case TCSETAW:   return TCSETAW;
 - case TCSETSW:   return TCSETSW;
 - case TCSETAF:   return TCSETAF;
 - case TCSETSF:   return TCSETSF;
 - case TCSBRK:return TCSBRK;
 - case TCXONC:return TCXONC;
 - case TCFLSH:return TCFLSH;
 - case TIOCGSID:  return TIOCGSID;
 -
 - case TIOCGETD:  return TIOCGETD;
 - case TIOCSETD:  return TIOCSETD;
 - case TIOCGWINSZ:return TIOCGWINSZ;
 - case TIOCSWINSZ:return TIOCSWINSZ;
 -
 - case TIOCMGET:  return TIOCMGET;
 - case TIOCMSET:  return TIOCMSET;
 - case TIOCMBIS:  return TIOCMBIS;
 - case TIOCMBIC:  return TIOCMBIC;
 -
 - /* from digi.h */
 - case DIGI_SETA: return DIGI_SETA;
 - case DIGI_SETAW:return DIGI_SETAW;
 - case DIGI_SETAF:return DIGI_SETAF;
 - case DIGI_SETFLOW:  return DIGI_SETFLOW;
 - case DIGI_SETAFLOW: return DIGI_SETAFLOW;
 - case DIGI_GETFLOW:  return DIGI_GETFLOW;
 - case DIGI_GETAFLOW: return DIGI_GETAFLOW;
 - case DIGI_GETA: return DIGI_GETA;
 - case DIGI_GEDELAY:  return DIGI_GEDELAY;
 - case DIGI_SEDELAY:  return DIGI_SEDELAY;
 - case DIGI_GETCUSTOMBAUD: return DIGI_GETCUSTOMBAUD;
 - case DIGI_SETCUSTOMBAUD: return DIGI_SETCUSTOMBAUD;
 - case TIOCMODG:  return TIOCMODG;
 - case TIOCMODS:  return TIOCMODS;
 - case TIOCSDTR:  return TIOCSDTR;
 - case TIOCCDTR:  return TIOCCDTR;
 -
 - default:return unknown;
 - }
 -}
 diff --git a/drivers/staging/dgnc/dgnc_utils.h 
 b/drivers/staging/dgnc/dgnc_utils.h
 index cebf601..1164c3a 100644
 --- a/drivers/staging/dgnc/dgnc_utils.h
 +++ b/drivers/staging/dgnc/dgnc_utils.h
 @@ -2,6 +2,5 @@
  #define __DGNC_UTILS_H
  
  int dgnc_ms_sleep(ulong ms);
 -char *dgnc_ioctl_name(int cmd);
  
  #endif
 


-- 
Thanks,
Giedrius
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] dgnc: Don't save boards in memory that have failed to initialize

[Giedrius Statkevičius]

Hi Greg,

On 2015.03.12 12:08, Greg KH wrote:
> On Mon, Mar 09, 2015 at 06:29:38PM +0200, Giedrius Statkevičius wrote:
>> Remove BOARD_FAILED and don't save dgnc_boards which failed to
>> initialize.
>>
>> Assign the result of kzalloc() to brd in dgnc_found_board() and only put
>> it in the dgnc_Board[] if it successfully initializes. Also, remove
>> BOARD_FAILED enum and all ifs that check for it. Finally, remove one
>> final place where state was set to BOARD_FAILED which was even redundant
>> before this patch.
>>
>> Signed-off-by: Giedrius Statkevičius 
>> ---
>> v2: Remove "brd = dgnc_Board[dgnc_NumBoards];" line which I forgot to do
>> in the first version
>>
>>  drivers/staging/dgnc/dgnc_driver.c | 20 ++--
>>  drivers/staging/dgnc/dgnc_driver.h |  3 +--
>>  drivers/staging/dgnc/dgnc_mgmt.c   |  5 +
>>  drivers/staging/dgnc/dgnc_tty.c|  8 
>>  4 files changed, 4 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/staging/dgnc/dgnc_driver.c 
>> b/drivers/staging/dgnc/dgnc_driver.c
>> index fa1ee79..075727d 100644
>> --- a/drivers/staging/dgnc/dgnc_driver.c
>> +++ b/drivers/staging/dgnc/dgnc_driver.c
>> @@ -401,8 +401,7 @@ static int dgnc_found_board(struct pci_dev *pdev, int id)
>>  unsigned long flags;
>>  
>>  /* get the board structure and prep it */
>> -dgnc_Board[dgnc_NumBoards] = kzalloc(sizeof(*brd), GFP_KERNEL);
>> -brd = dgnc_Board[dgnc_NumBoards];
>> +brd = kzalloc(sizeof(*brd), GFP_KERNEL);
> 
> You've done a great job here, but...
> 
> Yeah, sorry...
> 
> I really want to see this whole "static list of boards/cards" go away.
> There should not be any need for that in any in-kernel driver.  Your
> patch here is a sign that things are really wrong with this whole static
> array mess.
> 
> So could you do that instead?  I don't want to take patches around this
> whole "board state" mess anymore, as it should all not be needed at all.
> 
> If you need pointers on what needs to be done here, just let me know.
> 
> thanks,
> 
> greg k-h
> 

Hmm,
The only thing I thought of that is relevant is that the one big
file/ioctl that handles getting info of any board could be split into
many and each could return information about that own board
(DIGI_GET_NI_INFO => get non-intelligent state info and DIGI_GETBD =>
get board info). The last ioctl that returns number of boards could be
deduced from special files (of each board) in the system.

I don't know how we can avoid a list of boards because we create sysfs
files for each board, register each board with tty core and poll all the
boards each 20ms.

Could you elaborate more, give some more info?

-- 
Thanks,
Giedrius
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 10/10] mach-osiris-dvs.c : use "&&" (not "&") with bool operands

[Giedrius Statkevičius]

On 2015.03.13 16:31, Ameen Ali wrote:
> itwise AND ("&") was used
> in logical expressions with operands having "bool" type. Replaced
> bitwise AND operators with logical AND.
> 
> Signed-off-by : 
Always use checkpatch to check the .patch for these kinds of issues.
Also the Subject: is not properly formatted. Usually we do something
like:
subsystem/architecture: bla bla bla
You can look at the 'git log' for examples and format it similarly.

> ---
>  arch/arm/mach-s3c24xx/mach-osiris-dvs.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/mach-s3c24xx/mach-osiris-dvs.c 
> b/arch/arm/mach-s3c24xx/mach-osiris-dvs.c
> index ce2db23..e189836 100644
> --- a/arch/arm/mach-s3c24xx/mach-osiris-dvs.c
> +++ b/arch/arm/mach-s3c24xx/mach-osiris-dvs.c
> @@ -70,16 +70,16 @@ static int osiris_dvs_notify(struct notifier_block *nb,
>  
>   switch (val) {
>   case CPUFREQ_PRECHANGE:
> - if (old_dvs & !new_dvs ||
> - cur_dvs & !new_dvs) {
> + if (old_dvs && !new_dvs ||
> + cur_dvs && !new_dvs) {
>   pr_debug("%s: exiting dvs\n", __func__);
>   cur_dvs = false;
>   gpio_set_value(OSIRIS_GPIO_DVS, 1);
>   }
>   break;
>   case CPUFREQ_POSTCHANGE:
> - if (!old_dvs & new_dvs ||
> - !cur_dvs & new_dvs) {
> + if (!old_dvs && new_dvs ||
> + !cur_dvs && new_dvs) {
>   pr_debug("entering dvs\n");
>   cur_dvs = true;
>   gpio_set_value(OSIRIS_GPIO_DVS, 0);
> 


-- 
Thanks,
Giedrius
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/