AW: [PATCH] staging: gasket: Fix sizeof() in gasket_handle_ioctl()

[Walter Harms]

why not mark it as "Deprecated" and remove it with the next version ? Maybe 
soneone will wakeup ?

re,
 wh

Von: Greg Kroah-Hartman 
Gesendet: Dienstag, 9. März 2021 14:26:55
An: Dan Carpenter
Cc: Rob Springer; de...@driverdev.osuosl.org; kernel-janit...@vger.kernel.org; 
John Joseph; Simon Que; Richard Yeh; Todd Poynor
Betreff: Re: [PATCH] staging: gasket: Fix sizeof() in gasket_handle_ioctl()

On Fri, Jan 22, 2021 at 06:01:13PM +0300, Dan Carpenter wrote:
> The "gasket_dev->num_page_tables" variable is an int but this is copying
> sizeof(u64).  On 32 bit systems this would end up disclosing a kernel
> pointer to user space, but on 64 bit it copies zeroes from a struct
> hole.
>
> Fixes: 9a69f5087ccc ("drivers/staging: Gasket driver framework + Apex driver")
> Signed-off-by: Dan Carpenter 
> ---
> This is an API change.  Please review this carefully!  Another potential
> fix would be to make ->num_page_tables a long instead of an int.
>
>  drivers/staging/gasket/gasket_ioctl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Looks like this driver is dead, with no response from anyone from
Google.

Should I just delete it?  The goal of using normal apis and getting this
out of staging seems to have totally died, so it shouldn't even still be
living in the kernel tree.  Even if having it here actually finds
security issues that the authors missed like this :(

So, any objection to me deleting it?

thanks,

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

AW: [PATCH] staging: rtl8723bs: core: remove redundant zero'ing of counter variable k

[Walter Harms]

Von: Dan Carpenter 
Gesendet: Montag, 24. Februar 2020 12:27
An: Walter Harms
Cc: Colin King; Greg Kroah-Hartman; de...@driverdev.osuosl.org; 
kernel-janit...@vger.kernel.org; linux-ker...@vger.kernel.org
Betreff: Re: [PATCH] staging: rtl8723bs: core: remove redundant zero'ing of 
counter variable k

On Mon, Feb 24, 2020 at 11:07:55AM +, Walter Harms wrote:
> diff --git a/drivers/staging/rtl8723bs/core/rtw_efuse.c 
> b/drivers/staging/rtl8723bs/core/rtw_efuse.c
> index 3b8848182221..bdb6ff8aab7d 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_efuse.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_efuse.c
> @@ -244,10 +244,8 @@ u16Address)
> while (!(Bytetemp & 0x80)) {
> Bytetemp = rtw_read8(Adapter, EFUSE_CTRL+3);
> k++;
> -   if (k == 1000) {
> -   k = 0;
> +   if (k == 1000)
> break;
> -   }
>
> IMHO this is confusing to read, i suggest:
>
>  for(k=0;k<1000;k++) {
>   Bytetemp = rtw_read8(Adapter, EFUSE_CTRL+3);
>   if ( Bytetemp & 0x80 )
>break;
>   }
>

The problem with the original code is that the variable is named "k"
instead of "retry".  It should be:

do {
Bytetemp = rtw_read8(Adapter, EFUSE_CTRL+3);
} while (!(Bytetemp & 0x80)) && ++retry < 1000);

good point,
personally i try to avoid putting to much into braces, so i
would go for the additional if() but this is for the maintainer.


>  NTL is am wondering what will happen if k==1000
>  and Bytetemp is still invalid. Will rtw_read8() fail or
>  simply return invalid data ?

Yeah.  That was my thought reviewing this patch as well.

It should probably return 0xff on failure.

if (retry >= 1000)
return 0xff;

looks nice,

re,
 wh
regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

AW: [PATCH] staging: rtl8723bs: core: remove redundant zero'ing of counter variable k

[Walter Harms]

Von: kernel-janitors-ow...@vger.kernel.org 
 im Auftrag von Colin King 

Gesendet: Sonntag, 23. Februar 2020 16:28
An: Greg Kroah-Hartman; de...@driverdev.osuosl.org
Cc: kernel-janit...@vger.kernel.org; linux-ker...@vger.kernel.org
Betreff: [PATCH] staging: rtl8723bs: core: remove redundant zero'ing of counter 
variable k

From: Colin Ian King 

The zero'ing of counter variable k is redundant as it is never read
after breaking out of the while loop. Remove it.

Addresses-Coverity: ("Unused value")
Signed-off-by: Colin Ian King 
---
 drivers/staging/rtl8723bs/core/rtw_efuse.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/staging/rtl8723bs/core/rtw_efuse.c 
b/drivers/staging/rtl8723bs/core/rtw_efuse.c
index 3b8848182221..bdb6ff8aab7d 100644
--- a/drivers/staging/rtl8723bs/core/rtw_efuse.c
+++ b/drivers/staging/rtl8723bs/core/rtw_efuse.c
@@ -244,10 +244,8 @@ u16Address)
while (!(Bytetemp & 0x80)) {
Bytetemp = rtw_read8(Adapter, EFUSE_CTRL+3);
k++;
-   if (k == 1000) {
-   k = 0;
+   if (k == 1000)
break;
-   }

IMHO this is confusing to read, i suggest:

 for(k=0;k<1000;k++) {
  Bytetemp = rtw_read8(Adapter, EFUSE_CTRL+3);
  if ( Bytetemp & 0x80 )
   break;
  }

 NTL is am wondering what will happen if k==1000
 and Bytetemp is still invalid. Will rtw_read8() fail or
 simply return invalid data ?

 ym2c,
 re,
 wh
}
return rtw_read8(Adapter, EFUSE_CTRL);
} else
--
2.25.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: rtl8192u: fix indentation issue

[walter harms]

Am 14.11.2019 10:54, schrieb Colin King:
> From: Colin Ian King 
> 
> There is a block of statements that are indented
> too deeply, remove the extraneous tabs.
> 
> Signed-off-by: Colin Ian King 
> ---
>  drivers/staging/rtl8192u/r819xU_cmdpkt.c | 25 
>  1 file changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/staging/rtl8192u/r819xU_cmdpkt.c 
> b/drivers/staging/rtl8192u/r819xU_cmdpkt.c
> index e064f43fd8b6..bc98cdaf61ec 100644
> --- a/drivers/staging/rtl8192u/r819xU_cmdpkt.c
> +++ b/drivers/staging/rtl8192u/r819xU_cmdpkt.c
> @@ -169,19 +169,20 @@ static void cmdpkt_beacontimerinterrupt_819xusb(struct 
> net_device *dev)
>  {
>   struct r8192_priv *priv = ieee80211_priv(dev);
>   u16 tx_rate;
> - /* 87B have to S/W beacon for DTM encryption_cmn. */
> - if (priv->ieee80211->current_network.mode == IEEE_A ||
> - priv->ieee80211->current_network.mode == IEEE_N_5G ||
> - (priv->ieee80211->current_network.mode == IEEE_N_24G &&
> -  (!priv->ieee80211->pHTInfo->bCurSuppCCK))) {
> - tx_rate = 60;
> - DMESG("send beacon frame  tx rate is 6Mbpm\n");
> - } else {
> - tx_rate = 10;
> - DMESG("send beacon frame  tx rate is 1Mbpm\n");
> - }
>  
> - rtl819xusb_beacon_tx(dev, tx_rate); /* HW Beacon */
> + /* 87B have to S/W beacon for DTM encryption_cmn. */
> + if (priv->ieee80211->current_network.mode == IEEE_A ||
> + priv->ieee80211->current_network.mode == IEEE_N_5G ||
> + (priv->ieee80211->current_network.mode == IEEE_N_24G &&
> +  (!priv->ieee80211->pHTInfo->bCurSuppCCK))) {
> + tx_rate = 60;
> + DMESG("send beacon frame  tx rate is 6Mbpm\n");
> + } else {
> + tx_rate = 10;
> + DMESG("send beacon frame  tx rate is 1Mbpm\n");
> + }
> +
> + rtl819xusb_beacon_tx(dev, tx_rate); /* HW Beacon */
>  }
>  
>  
> /*-

this is hard to read in the first place.
Maybe using switch() here is better to read (untested example below).


switch(priv->ieee80211->current_network.mode) {

case IEEE_A:
case IEEE_N_5G:
 tx_rate = 60;
 break;
IEEE_N_24G:
 if ( !priv->ieee80211->pHTInfo->bCurSuppCCK )
tx_rate = 60;
 // fall truh

default:
tx_rate = 10;

}   

if (txrate == 60 )
 DMESG("send beacon frame  tx rate is 6Mbpm\n");
else if (txrate == 10 )
 DMESG("send beacon frame  tx rate is 1Mbpm\n");

JM2C

re,
 wh
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: rtl8192u: fix potential infinite loop because loop counter being too small

[walter harms]

Am 01.11.2019 15:51, schrieb Dan Carpenter:
> On Fri, Nov 01, 2019 at 02:26:04PM +, Colin King wrote:
>> From: Colin Ian King 
>>
>> Currently the for-loop counter i is a u8 however it is being checked
>> against a maximum value priv->ieee80211->LinkDetectInfo.SlotNum which is a
>> u16. Hence there is a potential wrap-around of counter i back to zero if
>> priv->ieee80211->LinkDetectInfo.SlotNum is greater than 255.  Fix this by
>> making i a u16.
>>
>> Addresses-Coverity: ("Infinite loop")
>> Fixes: 8fc8598e61f6 ("Staging: Added Realtek rtl8192u driver to staging")
>> Signed-off-by: Colin Ian King 
>> ---
>>  drivers/staging/rtl8192u/r8192U_core.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/rtl8192u/r8192U_core.c 
>> b/drivers/staging/rtl8192u/r8192U_core.c
>> index 48f1591ed5b4..fd91b7c5ca81 100644
>> --- a/drivers/staging/rtl8192u/r8192U_core.c
>> +++ b/drivers/staging/rtl8192u/r8192U_core.c
>> @@ -3210,7 +3210,7 @@ static void rtl819x_update_rxcounts(struct r8192_priv 
>> *priv, u32 *TotalRxBcnNum,
>>   u32 *TotalRxDataNum)
>>  {
>>  u16 SlotIndex;
>> -u8  i;
>> +u16 i;
> 
> The iterator "i" should just be an int unless we know that it needs to
> be an unsigned long long.
> 

+1

i think we can spare the 2byte. ppl expect int and will get confused (as shown 
here).

re,
 wh


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] staging: iio: tsl2x7x: clean up limit checks

[walter harms]

Am 08.09.2017 12:53, schrieb Dan Carpenter:
> The background of this code is that we can either use the default
> tables or load our own table with sysfs.  The default tables are three
> element arrays of struct tsl2x7x_lux.  If we load the table with sysfs
> then we can have as many as nine elements.  Which ever way we do it, the
> last element is always zeroed out.
> 
> The most interesting part of this patch is in the
> in_illuminance0_lux_table_show() function.  We were using the wrong
> limit, "TSL2X7X_MAX_LUX_TABLE_SIZE * 3", when it should have been just
> "TSL2X7X_MAX_LUX_TABLE_SIZE".  This creates a static checker warning
> that we are going of of bounds.  However, since the last element is
> always zeroed out, that means we hit the break statement and the code
> works correctly despite the wrong limit check.
> 
> I made several related readability changes.  The most notable that I
> changed the MAX_DEFAULT_TABLE_BYTES define which was:
> 
> #define MAX_DEFAULT_TABLE_BYTES (sizeof(int) * TSL2X7X_MAX_LUX_TABLE_SIZE)
> 
> I renamed the define to TSL2X7X_DEFAULT_TABLE_BYTES because it's not the
> max size, it's the only size.  Also the size should really be expressed
> as sizeof(struct tsl2x7x_lux) * 3.  In other words, 12 * 3 instead of
> 4 * 9.  It's 36 bytes either way, so this doesn't change the behavior.
> 
> Finally, I created the TSL2X7X_DEF_LUX_TABLE_SZ define instead of using
> the magic number 3.  I declared the default tables using that define
> to hopefully signal to future programmers that if they want to use a
> different size they have to update all the related code.
> 
> Signed-off-by: Dan Carpenter 
> 
> diff --git a/drivers/staging/iio/light/tsl2x7x.h 
> b/drivers/staging/iio/light/tsl2x7x.h
> index ecae92211216..a216c6943a84 100644
> --- a/drivers/staging/iio/light/tsl2x7x.h
> +++ b/drivers/staging/iio/light/tsl2x7x.h
> @@ -23,10 +23,6 @@
>  #define __TSL2X7X_H
>  #include 
>  
> -/* Max number of segments allowable in LUX table */
> -#define TSL2X7X_MAX_LUX_TABLE_SIZE   9
> -#define MAX_DEFAULT_TABLE_BYTES (sizeof(int) * TSL2X7X_MAX_LUX_TABLE_SIZE)
> -
>  struct iio_dev;
>  
>  struct tsl2x7x_lux {
> @@ -35,6 +31,13 @@ struct tsl2x7x_lux {
>   unsigned int ch1;
>  };
>  
> +/* Max number of segments allowable in LUX table */
> +#define TSL2X7X_MAX_LUX_TABLE_SIZE   9
> +/* The default LUX tables all have 3 elements.  */
> +#define TSL2X7X_DEF_LUX_TABLE_SZ 3
> +#define TSL2X7X_DEFAULT_TABLE_BYTES (sizeof(struct tsl2x7x_lux) * \
> +  TSL2X7X_DEF_LUX_TABLE_SZ)
> +
>  /**
>   * struct tsl2x7x_default_settings - power on defaults unless
>   *   overridden by platform data.
> diff --git a/drivers/staging/iio/light/tsl2x7x.c 
> b/drivers/staging/iio/light/tsl2x7x.c
> index 786e93f16ce9..b3a9efecf536 100644
> --- a/drivers/staging/iio/light/tsl2x7x.c
> +++ b/drivers/staging/iio/light/tsl2x7x.c
> @@ -192,25 +192,25 @@ struct tsl2X7X_chip {
>  };
>  
>  /* Different devices require different coefficents */
> -static const struct tsl2x7x_lux tsl2x71_lux_table[] = {
> +static const struct tsl2x7x_lux tsl2x71_lux_table[TSL2X7X_DEF_LUX_TABLE_SZ] 
> = {
>   { 14461,   611,   1211 },
>   { 18540,   352,623 },
>   { 0, 0,  0 },
>  };
>  
> -static const struct tsl2x7x_lux tmd2x71_lux_table[] = {
> +static const struct tsl2x7x_lux tmd2x71_lux_table[TSL2X7X_DEF_LUX_TABLE_SZ] 
> = {
>   { 11635,   115,256 },
>   { 15536,87,179 },
>   { 0, 0,  0 },
>  };
>  
> -static const struct tsl2x7x_lux tsl2x72_lux_table[] = {
> +static const struct tsl2x7x_lux tsl2x72_lux_table[TSL2X7X_DEF_LUX_TABLE_SZ] 
> = {
>   { 14013,   466,   917 },
>   { 18222,   310,   552 },
>   { 0, 0, 0 },
>  };
>  
> -static const struct tsl2x7x_lux tmd2x72_lux_table[] = {
> +static const struct tsl2x7x_lux tmd2x72_lux_table[TSL2X7X_DEF_LUX_TABLE_SZ] 
> = {
>   { 13218,   130,   262 },
>   { 17592,   92,169 },
>   { 0, 0, 0 },
> @@ -530,7 +530,7 @@ static void tsl2x7x_defaults(struct tsl2X7X_chip *chip)
>   else
>   memcpy(chip->tsl2x7x_device_lux,
>   (struct tsl2x7x_lux *)tsl2x7x_default_lux_table_group[chip->id],
> - MAX_DEFAULT_TABLE_BYTES);
> + TSL2X7X_DEFAULT_TABLE_BYTES);
>  }
>  
>  /**
> @@ -1113,7 +1113,7 @@ static ssize_t in_illuminance0_lux_table_show(struct 
> device *dev,
>   int i = 0;
>   int offset = 0;
>  
> - while (i < (TSL2X7X_MAX_LUX_TABLE_SIZE * 3)) {
> + while (i < TSL2X7X_MAX_LUX_TABLE_SIZE) {
>   offset += snprintf(buf + offset, PAGE_SIZE, "%u,%u,%u,",
>   chip->tsl2x7x_device_lux[i].ratio,
>   chip->tsl2x7x_device_lux[i].ch0,

Is that TSL2X7X_MAX_LUX_TABLE_SIZE needed at all ? because:

if 

Re: [PATCH] staging: rtl8192u: fix incorrect mask when calculating TxPowerLevelCCK

[walter harms]

Am 05.09.2017 18:32, schrieb Colin King:
> From: Colin Ian King 
> 
> The mask of 0xff and right shift of 8 bits on ret always results in
> a value of 0 for TxPowerLevelCCK.  I believe this should be a mask of
> 0xff00, however I do not have the hardware at hand to test this out,
> so there is a distinct possibility I may be wrong on this.
> 
> Detected by CoverityScan CID#1357110 ("Operands don't affect result")
> 
> Signed-off-by: Colin Ian King 
> ---
>  drivers/staging/rtl8192u/r8192U_core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/rtl8192u/r8192U_core.c 
> b/drivers/staging/rtl8192u/r8192U_core.c
> index 46b3f19e0878..ecc887636173 100644
> --- a/drivers/staging/rtl8192u/r8192U_core.c
> +++ b/drivers/staging/rtl8192u/r8192U_core.c
> @@ -2510,7 +2510,7 @@ static int rtl8192_read_eeprom_info(struct net_device 
> *dev)
>   ret = eprom_read(dev, (EEPROM_TxPwIndex_CCK >> 
> 1));
>   if (ret < 0)
>   return ret;
> - priv->EEPROMTxPowerLevelCCK = ((u16)ret & 0xff) 
> >> 8;
> + priv->EEPROMTxPowerLevelCCK = ((u16)ret & 
> 0xff00) >> 8;

Is there any need for the mask ?
(u16) will already reduce ret to 16 bit, the >>8 will shift the lower bits into 
nirwana

re,
 wh

>   } else
>   priv->EEPROMTxPowerLevelCCK = 0x10;
>   RT_TRACE(COMP_EPROM, "CCK Tx Power Levl: 0x%02x\n", 
> priv->EEPROMTxPowerLevelCCK);
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: pi433: fix bugs in register abstraction of rf69 chip

[walter harms]

Am 20.07.2017 18:08, schrieb Marcus Wolf:
> Hi Walter,
>  
> since the construction
> 
> WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & ~MASK_LNA_GAIN) |
> LNA_GAIN_MAX_MINUS_6) )
> aka
> WRITE_REG(regname, ( (READ_REG(regname) & ~regmask  ) | vale
>) )
> 
> is used nearly everywhere, I think, about using a more gneric macro like
> 
> #define READ_MODIFY_WRITE(reg, mask, value) /
> WRITE_REG(reg, ((READ_REG(reg) & ~mask) | vale ))
> 
> 
> What do you think about it?

Yep, good idea.
personally i prefer functions because for a linux kernel the scope of
DEFINE is a bit large.

Small functions tend to be inlined by the compiler, so there is no speed
disadvatage. But this is a matter of taste and the maintainer has to maintain
whatever happens.

just my 2 cents
re,
 wh



> 
> Cheers,
> 
> Marcus
> 
>> walter harms <wha...@bfs.de> hat am 20. Juli 2017 um 14:22 geschrieben:
>>
>>
>>
>>
>> Am 19.07.2017 20:18, schrieb Wolf Entwicklungen:
>>> Bugfixes for rf69_set_modulation, rf69_set_deviation, rf69_set_lna_gain and
>>> rf69_get_lna_gain
>>> The fixes are cross-checked with the datasheet of the rfm69cw
>>>
>>> Fixes: 874bcba65f9a ("staging: pi433: New driver")
>>> Signed-off-by: Marcus Wolf <li...@wolf-entwicklungen.de>
>>>
>>> diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
>>> --- a/drivers/staging/pi433/rf69.c
>>> +++ b/drivers/staging/pi433/rf69.c
>>> @@ -101,7 +101,7 @@ int rf69_set_modulation(struct spi_device *spi, enum
>>> modulation modulation)
>>>
>>> currentValue = READ_REG(REG_DATAMODUL);
>>>
>>> - switch (currentValue & MASK_DATAMODUL_MODULATION_TYPE >> 3) // TODO
>>> improvement: change 3 to define
>>> + switch (currentValue & MASK_DATAMODUL_MODULATION_TYPE)
>>> {
>>> case DATAMODUL_MODULATION_TYPE_OOK: return OOK;
>>> case DATAMODUL_MODULATION_TYPE_FSK: return FSK;
>>> @@ -203,7 +203,7 @@ int rf69_set_deviation(struct spi_device *spi, u32
>>> deviation)
>>> lsb = (f_reg&0xff);
>>>
>>> // check msb
>>> - if (msb & !FDEVMASB_MASK)
>>> + if (msb & ~FDEVMASB_MASK)
>>> {
>>> dev_dbg(>dev, "set_deviation: err in calc of msb");
>>> INVALID_PARAM;
>>> @@ -366,13 +366,13 @@ int rf69_set_lna_gain(struct spi_device *spi, enum
>>> lnaGain lnaGain)
>>> #endif
>>>
>>> switch(lnaGain) {
>>> - case automatic: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) &
>>> ~MASK_LNA_GAIN) & LNA_GAIN_AUTO) );
>>> - case max: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & ~MASK_LNA_GAIN)
>>> & LNA_GAIN_MAX) );
>>> - case maxMinus6: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) &
>>> ~MASK_LNA_GAIN) & LNA_GAIN_MAX_MINUS_6) );
>>> - case maxMinus12: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) &
>>> ~MASK_LNA_GAIN) & LNA_GAIN_MAX_MINUS_12) );
>>> - case maxMinus24: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) &
>>> ~MASK_LNA_GAIN) & LNA_GAIN_MAX_MINUS_24) );
>>> - case maxMinus36: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) &
>>> ~MASK_LNA_GAIN) & LNA_GAIN_MAX_MINUS_36) );
>>> - case maxMinus48: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) &
>>> ~MASK_LNA_GAIN) & LNA_GAIN_MAX_MINUS_48) );
>>> + case automatic: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) &
>>> ~MASK_LNA_GAIN) | LNA_GAIN_AUTO) );
>>> + case max: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & ~MASK_LNA_GAIN)
>>> | LNA_GAIN_MAX) );
>>> + case maxMinus6: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) &
>>> ~MASK_LNA_GAIN) | LNA_GAIN_MAX_MINUS_6) );
>>> + case maxMinus12: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) &
>>> ~MASK_LNA_GAIN) | LNA_GAIN_MAX_MINUS_12) );
>>> + case maxMinus24: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) &
>>> ~MASK_LNA_GAIN) | LNA_GAIN_MAX_MINUS_24) );
>>> + case maxMinus36: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) &
>>> ~MASK_LNA_GAIN) | LNA_GAIN_MAX_MINUS_36) );
>>> + case maxMinus48: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) &
>>> ~MASK_LNA_GAIN) | LNA_GAIN_MAX_MINUS_48) );
>>> default: INVALID_PARAM;
>>> }
>>> }
>>
>>
>> looks resonable,
>> some nitpicking: perhaps you can can improve readability by using a helper
>> like:
>>
>> static int setstbit(int arg)
>>

Re: [PATCH] staging: pi433: fix bugs in register abstraction of rf69 chip

[walter harms]

Am 19.07.2017 20:18, schrieb Wolf Entwicklungen:
> Bugfixes for rf69_set_modulation, rf69_set_deviation, rf69_set_lna_gain and 
> rf69_get_lna_gain
> The fixes are cross-checked with the datasheet of the rfm69cw
> 
> Fixes: 874bcba65f9a ("staging: pi433: New driver")
> Signed-off-by: Marcus Wolf 
> 
> diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
> --- a/drivers/staging/pi433/rf69.c
> +++ b/drivers/staging/pi433/rf69.c
> @@ -101,7 +101,7 @@ int rf69_set_modulation(struct spi_device *spi, enum 
> modulation modulation)
> 
>   currentValue = READ_REG(REG_DATAMODUL);
> 
> - switch (currentValue & MASK_DATAMODUL_MODULATION_TYPE >> 3)  // TODO 
> improvement: change 3 to define
> + switch (currentValue & MASK_DATAMODUL_MODULATION_TYPE)
>   {
>   case DATAMODUL_MODULATION_TYPE_OOK: return OOK;
>   case DATAMODUL_MODULATION_TYPE_FSK: return FSK;
> @@ -203,7 +203,7 @@ int rf69_set_deviation(struct spi_device *spi, u32 
> deviation)
>   lsb = (f_reg&0xff);
> 
>   // check msb
> - if (msb & !FDEVMASB_MASK)
> + if (msb & ~FDEVMASB_MASK)
>   {
>   dev_dbg(>dev, "set_deviation: err in calc of msb");
>   INVALID_PARAM;
> @@ -366,13 +366,13 @@ int rf69_set_lna_gain(struct spi_device *spi, enum 
> lnaGain lnaGain)
>   #endif
> 
>   switch(lnaGain) {
> - case automatic:  return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & 
> ~MASK_LNA_GAIN) & LNA_GAIN_AUTO) );
> - case max:return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & 
> ~MASK_LNA_GAIN) & LNA_GAIN_MAX) );
> - case maxMinus6:  return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & 
> ~MASK_LNA_GAIN) & LNA_GAIN_MAX_MINUS_6) );
> - case maxMinus12: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & 
> ~MASK_LNA_GAIN) & LNA_GAIN_MAX_MINUS_12) );
> - case maxMinus24: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & 
> ~MASK_LNA_GAIN) & LNA_GAIN_MAX_MINUS_24) );
> - case maxMinus36: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & 
> ~MASK_LNA_GAIN) & LNA_GAIN_MAX_MINUS_36) );
> - case maxMinus48: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & 
> ~MASK_LNA_GAIN) & LNA_GAIN_MAX_MINUS_48) );
> + case automatic:  return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & 
> ~MASK_LNA_GAIN) | LNA_GAIN_AUTO) );
> + case max:return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & 
> ~MASK_LNA_GAIN) | LNA_GAIN_MAX) );
> + case maxMinus6:  return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & 
> ~MASK_LNA_GAIN) | LNA_GAIN_MAX_MINUS_6) );
> + case maxMinus12: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & 
> ~MASK_LNA_GAIN) | LNA_GAIN_MAX_MINUS_12) );
> + case maxMinus24: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & 
> ~MASK_LNA_GAIN) | LNA_GAIN_MAX_MINUS_24) );
> + case maxMinus36: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & 
> ~MASK_LNA_GAIN) | LNA_GAIN_MAX_MINUS_36) );
> + case maxMinus48: return WRITE_REG(REG_LNA, ( (READ_REG(REG_LNA) & 
> ~MASK_LNA_GAIN) | LNA_GAIN_MAX_MINUS_48) );
>   default: INVALID_PARAM;
>   }
>  }


looks resonable,
some nitpicking: perhaps you can can improve readability by using a helper like:

static int setstbit(int arg)
{
int get=READ_REG(REG_LNA) & ~MASK_LNA_GAIN;
return WRITE_REG( get| arg);

}

so this switch would be reduced to ...

case maxMinus6: return setstbit(LNA_GAIN_MAX_MINUS_6);

re,
 wh


> @@ -387,7 +387,7 @@ enum lnaGain rf69_get_lna_gain(struct spi_device *spi)
> 
>   currentValue = READ_REG(REG_LNA);
> 
> - switch (currentValue & MASK_LNA_CURRENT_GAIN >> 3)  // improvement: 
> change 3 to define
> + switch (currentValue & MASK_LNA_GAIN)
>   {
>   case LNA_GAIN_AUTO: return automatic;
>   case LNA_GAIN_MAX:  return max;
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: pi433: fix a precedence bug

[walter harms]

Am 19.07.2017 11:51, schrieb Dan Carpenter:
> We had intended to do the mask first and then the shift.  Shift has
> higher precedence so we need to add parenthesis.
> 
> Fixes: 874bcba65f9a ("staging: pi433: New driver")
> Signed-off-by: Dan Carpenter 
> 
> diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
> index e391ce777bc7..5089449bf775 100644
> --- a/drivers/staging/pi433/rf69.c
> +++ b/drivers/staging/pi433/rf69.c
> @@ -387,7 +387,7 @@ enum lnaGain rf69_get_lna_gain(struct spi_device *spi)
>  
>   currentValue = READ_REG(REG_LNA);


is currentValue used again later ?

iff not (IMHO) it would be more readable to use

currentValue &= MASK_LNA_CURRENT_GAIN

here.

and
 currentValue >>3

inside the switch()
>  
> - switch (currentValue & MASK_LNA_CURRENT_GAIN >> 3)  // improvement: 
> change 3 to define
> + switch ((currentValue & MASK_LNA_CURRENT_GAIN) >> 3)  // improvement: 
> change 3 to define
>   {
>   case LNA_GAIN_AUTO: return automatic;
>   case LNA_GAIN_MAX:  return max;

note:
and instead to add an artificial define for 3, it would be simpler to change 
the  LNA_GAIN_AUTO/MAX
to the "real" values and drop the >>3 for good.

just my 2 cents,

re,
 wh

> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: speakup: make function ser_to_dev static

[walter harms]

Am 28.06.2017 15:13, schrieb Colin King:
> From: Colin Ian King 
> 
> The helper function ser_to_dev does not need to be in global scope, so
> make it static.
> 
> Cleans up sparse warning:
> "warning: symbol 'ser_to_dev' was not declared. Should it be static?"
> 
> Signed-off-by: Colin Ian King 
> ---
>  drivers/staging/speakup/spk_ttyio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/speakup/spk_ttyio.c 
> b/drivers/staging/speakup/spk_ttyio.c
> index 442f191a017e..ed8e96b06ead 100644
> --- a/drivers/staging/speakup/spk_ttyio.c
> +++ b/drivers/staging/speakup/spk_ttyio.c
> @@ -21,7 +21,7 @@ struct spk_ldisc_data {
>  static struct spk_synth *spk_ttyio_synth;
>  static struct tty_struct *speakup_tty;
>  
> -int ser_to_dev(int ser, dev_t *dev_no)
> +static int ser_to_dev(int ser, dev_t *dev_no)
>  {
>   if (ser < 0 || ser > (255 - 64)) {
>   pr_err("speakup: Invalid ser param. Must be between 0 and 191 
> inclusive.\n");


Is there any remark, why the programmer decided to use (255 - 64) instead of 
191 ?

re,
 wh
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] atomisp: ensure that status values > 7 are reported as errors

[walter harms]

Am 06.06.2017 18:30, schrieb Colin King:
> From: Colin Ian King 
> 
> The current code checks if a status value is greater than 7 and
> sets the status string as "ERROR" and then over writes the
> string based on the bottom 3 bits of the value. Instead, fix this by
> only checking on the bottom 3 bits of the value if the value is less
> than 8.
> 
> Detected by CoverityScan, CID#1416607 ("Unused value")
> 
> Signed-off-by: Colin Ian King 
> ---
>  .../css2400/runtime/debug/src/ia_css_debug.c   | 141 
> +++--
>  1 file changed, 72 insertions(+), 69 deletions(-)
> 
> diff --git 
> a/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/debug/src/ia_css_debug.c
>  
> b/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/debug/src/ia_css_debug.c
> index bcc0d464084f..80d6fe29f30d 100644
> --- 
> a/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/debug/src/ia_css_debug.c
> +++ 
> b/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/debug/src/ia_css_debug.c
> @@ -765,28 +765,29 @@ static void 
> debug_print_if_state(input_formatter_state_t *state, const char *id)
>  
>   val = state->fsm_sync_status;
>  
> - if (val > 7)
> + if (val > 7) {
>   fsm_sync_status_str = "ERROR";
> -
> - switch (val & 0x7) {
> - case 0:
> - fsm_sync_status_str = "idle";
> - break;
> - case 1:
> - fsm_sync_status_str = "request frame";
> - break;
> - case 2:
> - fsm_sync_status_str = "request lines";
> - break;
> - case 3:
> - fsm_sync_status_str = "request vectors";
> - break;
> - case 4:
> - fsm_sync_status_str = "send acknowledge";
> - break;
> - default:
> - fsm_sync_status_str = "unknown";
> - break;
> + } else {
> + switch (val & 0x7) {
> + case 0:
> + fsm_sync_status_str = "idle";
> + break;
> + case 1:
> + fsm_sync_status_str = "request frame";
> + break;
> + case 2:
> + fsm_sync_status_str = "request lines";
> + break;
> + case 3:
> + fsm_sync_status_str = "request vectors";
> + break;
> + case 4:
> + fsm_sync_status_str = "send acknowledge";
> + break;
> + default:
> + fsm_sync_status_str = "unknown";
> + break;
> + }
>   }
>  
>   ia_css_debug_dtrace(2, "\t\t%-32s: (0x%X: %s)\n",
> @@ -799,34 +800,35 @@ static void 
> debug_print_if_state(input_formatter_state_t *state, const char *id)
>  
>   val = state->fsm_crop_status;
>  
> - if (val > 7)
> + if (val > 7) {
>   fsm_crop_status_str = "ERROR";
> -
> - switch (val & 0x7) {
> - case 0:
> - fsm_crop_status_str = "idle";
> - break;
> - case 1:
> - fsm_crop_status_str = "wait line";
> - break;
> - case 2:
> - fsm_crop_status_str = "crop line";
> - break;
> - case 3:
> - fsm_crop_status_str = "crop pixel";
> - break;
> - case 4:
> - fsm_crop_status_str = "pass pixel";
> - break;
> - case 5:
> - fsm_crop_status_str = "pass line";
> - break;
> - case 6:
> - fsm_crop_status_str = "lost line";
> - break;
> - default:
> - fsm_crop_status_str = "unknown";
> - break;
> + } else {
> + switch (val & 0x7) {
> + case 0:
> + fsm_crop_status_str = "idle";
> + break;
> + case 1:
> + fsm_crop_status_str = "wait line";
> + break;
> + case 2:
> + fsm_crop_status_str = "crop line";
> + break;
> + case 3:
> + fsm_crop_status_str = "crop pixel";
> + break;
> + case 4:
> + fsm_crop_status_str = "pass pixel";
> + break;
> + case 5:
> + fsm_crop_status_str = "pass line";
> + break;
> + case 6:
> + fsm_crop_status_str = "lost line";
> + break;
> + default:
> + fsm_crop_status_str = "unknown";
> + break;
> + }


why not:
case 7:
fsm_crop_status_str = "unknown";
break;

default:
fsm_crop_status_str = "ERROR";
break;


looks more straigth to me..
same below.

less execptions, less errors

re,
 wh

> 

Re: [PATCH 2/2] staging/atomisp: putting NULs in the wrong place

[walter harms]

Am 15.05.2017 12:01, schrieb Dan Carpenter:
> We're putting the NUL terminators one space beyond where they belong.
> This doesn't show up in testing because all but the callers put a NUL in
> the correct place themselves.  LOL.  It causes a static checker warning
> about buffer overflows.
> 
> Fixes: a49d25364dfb ("staging/atomisp: Add support for the Intel IPU v2")
> Signed-off-by: Dan Carpenter 
> 
> diff --git 
> a/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_include/string_support.h
>  
> b/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_include/string_support.h
> index 74b5a1c7ac9a..c53241a7a281 100644
> --- 
> a/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_include/string_support.h
> +++ 
> b/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_include/string_support.h
> @@ -117,7 +117,7 @@ STORAGE_CLASS_INLINE int strncpy_s(
>  
>   /* dest_str is big enough for the len */
>   strncpy(dest_str, src_str, len);
> - dest_str[len+1] = '\0';
> + dest_str[len] = '\0';
>   return 0;
>  }
>  
> @@ -157,7 +157,7 @@ STORAGE_CLASS_INLINE int strcpy_s(
>  
>   /* dest_str is big enough for the len */
>   strncpy(dest_str, src_str, len);
> - dest_str[len+1] = '\0';
> + dest_str[len] = '\0';
>   return 0;
>  }
>  

can this strcpy_s() replaced with strlcpy ?

re,
 wh


> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] staging: atomisp: use local variable to reduce the number of reference

[walter harms]

Am 30.03.2017 08:25, schrieb Daeseok Youn:
> Define new local variable to reduce the number of reference.
> The new local variable is added to save the addess of dfs
> and used in atomisp_freq_scaling() function.
> 
> Signed-off-by: Daeseok Youn 
> ---
>  .../media/atomisp/pci/atomisp2/atomisp_cmd.c   | 37 
> --
>  1 file changed, 20 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c 
> b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
> index eebfccd..d76a95c 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
> @@ -251,6 +251,7 @@ int atomisp_freq_scaling(struct atomisp_device *isp,
>  {
>   /* FIXME! Only use subdev[0] status yet */
>   struct atomisp_sub_device *asd = >asd[0];
> + const struct atomisp_dfs_config *dfs;
>   unsigned int new_freq;
>   struct atomisp_freq_scaling_rule curr_rules;
>   int i, ret;
> @@ -268,20 +269,22 @@ int atomisp_freq_scaling(struct atomisp_device *isp,
>   ATOMISP_USE_YUVPP(asd))
>   isp->dfs = _config_cht_soc;
>  
> - if (isp->dfs->lowest_freq == 0 || isp->dfs->max_freq_at_vmin == 0 ||
> - isp->dfs->highest_freq == 0 || isp->dfs->dfs_table_size == 0 ||
> - !isp->dfs->dfs_table) {
> + dfs = isp->dfs;
> +
> + if (dfs->lowest_freq == 0 || dfs->max_freq_at_vmin == 0 ||
> + dfs->highest_freq == 0 || dfs->dfs_table_size == 0 ||
> + !dfs->dfs_table) {
>   dev_err(isp->dev, "DFS configuration is invalid.\n");
>   return -EINVAL;
>   }
>  
>   if (mode == ATOMISP_DFS_MODE_LOW) {
> - new_freq = isp->dfs->lowest_freq;
> + new_freq = dfs->lowest_freq;
>   goto done;
>   }
>  
>   if (mode == ATOMISP_DFS_MODE_MAX) {
> - new_freq = isp->dfs->highest_freq;
> + new_freq = dfs->highest_freq;
>   goto done;
>   }
>  
> @@ -307,26 +310,26 @@ int atomisp_freq_scaling(struct atomisp_device *isp,
>   }
>  
>   /* search for the target frequency by looping freq rules*/
> - for (i = 0; i < isp->dfs->dfs_table_size; i++) {
> - if (curr_rules.width != isp->dfs->dfs_table[i].width &&
> - isp->dfs->dfs_table[i].width != ISP_FREQ_RULE_ANY)
> + for (i = 0; i < dfs->dfs_table_size; i++) {
> + if (curr_rules.width != dfs->dfs_table[i].width &&
> + dfs->dfs_table[i].width != ISP_FREQ_RULE_ANY)
>   continue;
> - if (curr_rules.height != isp->dfs->dfs_table[i].height &&
> - isp->dfs->dfs_table[i].height != ISP_FREQ_RULE_ANY)
> + if (curr_rules.height != dfs->dfs_table[i].height &&
> + dfs->dfs_table[i].height != ISP_FREQ_RULE_ANY)
>   continue;
> - if (curr_rules.fps != isp->dfs->dfs_table[i].fps &&
> - isp->dfs->dfs_table[i].fps != ISP_FREQ_RULE_ANY)
> + if (curr_rules.fps != dfs->dfs_table[i].fps &&
> + dfs->dfs_table[i].fps != ISP_FREQ_RULE_ANY)
>   continue;
> - if (curr_rules.run_mode != isp->dfs->dfs_table[i].run_mode &&
> - isp->dfs->dfs_table[i].run_mode != ISP_FREQ_RULE_ANY)
> + if (curr_rules.run_mode != dfs->dfs_table[i].run_mode &&
> + dfs->dfs_table[i].run_mode != ISP_FREQ_RULE_ANY)
>   continue;
>   break;
>   }

>  
> - if (i == isp->dfs->dfs_table_size)
> - new_freq = isp->dfs->max_freq_at_vmin;
> + if (i == dfs->dfs_table_size)
> + new_freq = dfs->max_freq_at_vmin;
>   else
> - new_freq = isp->dfs->dfs_table[i].isp_freq;
> + new_freq = dfs->dfs_table[i].isp_freq;
>  

you can eliminate the last block by setting

 new_freq = dfs->max_freq_at_vmin;

  for(i=0;) {

new_freq = dfs->dfs_table[i].isp_freq;
break;
}

unfortunately i have no good idea how to make the loop more readable.


re,
 wh


>  done:
>   dev_dbg(isp->dev, "DFS target frequency=%d.\n", new_freq);
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/4] staging: atomisp: simplify if statement in atomisp_get_sensor_fps()

[walter harms]

Am 20.03.2017 13:51, schrieb DaeSeok Youn:
> 2017-03-20 21:04 GMT+09:00 walter harms <wha...@bfs.de>:
>>
>>
>> Am 20.03.2017 11:59, schrieb Daeseok Youn:
>>> If v4l2_subdev_call() gets the global frame interval values,
>>> it returned 0 and it could be checked whether numerator is zero or not.
>>>
>>> If the numerator is not zero, the fps could be calculated in this function.
>>> If not, it just returns 0.
>>>
>>> Signed-off-by: Daeseok Youn <daeseok.y...@gmail.com>
>>> ---
>>>  .../media/atomisp/pci/atomisp2/atomisp_cmd.c   | 22 
>>> ++
>>>  1 file changed, 10 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c 
>>> b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
>>> index 8bdb224..6bdd19e 100644
>>> --- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
>>> +++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
>>> @@ -153,20 +153,18 @@ struct atomisp_acc_pipe *atomisp_to_acc_pipe(struct 
>>> video_device *dev)
>>>
>>>  static unsigned short atomisp_get_sensor_fps(struct atomisp_sub_device 
>>> *asd)
>>>  {
>>> - struct v4l2_subdev_frame_interval frame_interval;
>>> + struct v4l2_subdev_frame_interval fi;
>>>   struct atomisp_device *isp = asd->isp;
>>> - unsigned short fps;
>>>
>>> - if (v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
>>> - video, g_frame_interval, _interval)) {
>>> - fps = 0;
>>> - } else {
>>> - if (frame_interval.interval.numerator)
>>> - fps = frame_interval.interval.denominator /
>>> - frame_interval.interval.numerator;
>>> - else
>>> - fps = 0;
>>> - }
>>> + unsigned short fps = 0;
>>> + int ret;
>>> +
>>> + ret = v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
>>> +video, g_frame_interval, );
>>> +
>>> + if (!ret && fi.interval.numerator)
>>> + fps = fi.interval.denominator / fi.interval.numerator;
>>> +
>>>   return fps;
>>>  }
>>
>>
>>
>> do you need to check ret at all ? if an error occurs can 
>> fi.interval.numerator
>> be something else than 0 ?
> the return value from the v4l2_subdev_call() function is zero when it
> is done without any error. and also I checked
> the ret value whether is 0 or not. if the ret is 0 then the value of
> numerator should be checked to avoid for dividing by 0.
>>
>> if ret is an ERRNO it would be wise to return ret not fps, but this may 
>> require
>> changes at other places also.
> hmm.., yes, you are right. but I think it is ok because the
> atomisp_get_sensor_fps() function is needed to get fps value.
> (originally, zero or calculated fps value was returned.)

maybe its better to divide this in:
if (ret)
   return 0; // error case

return (fi.interval.numerator>0)?fi.interval.denominator / 
fi.interval.numerator:0;

So there is a chance that someone will a) understand and b) fix the error 
return.

re,
 wh

> 
>>
>> re,
>>  wh
>>
>>>
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/4] staging: atomisp: simplify if statement in atomisp_get_sensor_fps()

[walter harms]

Am 20.03.2017 11:59, schrieb Daeseok Youn:
> If v4l2_subdev_call() gets the global frame interval values,
> it returned 0 and it could be checked whether numerator is zero or not.
> 
> If the numerator is not zero, the fps could be calculated in this function.
> If not, it just returns 0.
> 
> Signed-off-by: Daeseok Youn 
> ---
>  .../media/atomisp/pci/atomisp2/atomisp_cmd.c   | 22 
> ++
>  1 file changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c 
> b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
> index 8bdb224..6bdd19e 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
> @@ -153,20 +153,18 @@ struct atomisp_acc_pipe *atomisp_to_acc_pipe(struct 
> video_device *dev)
>  
>  static unsigned short atomisp_get_sensor_fps(struct atomisp_sub_device *asd)
>  {
> - struct v4l2_subdev_frame_interval frame_interval;
> + struct v4l2_subdev_frame_interval fi;
>   struct atomisp_device *isp = asd->isp;
> - unsigned short fps;
>  
> - if (v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
> - video, g_frame_interval, _interval)) {
> - fps = 0;
> - } else {
> - if (frame_interval.interval.numerator)
> - fps = frame_interval.interval.denominator /
> - frame_interval.interval.numerator;
> - else
> - fps = 0;
> - }
> + unsigned short fps = 0;
> + int ret;
> +
> + ret = v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
> +video, g_frame_interval, );
> +
> + if (!ret && fi.interval.numerator)
> + fps = fi.interval.denominator / fi.interval.numerator;
> +
>   return fps;
>  }



do you need to check ret at all ? if an error occurs can fi.interval.numerator
be something else than 0 ?

if ret is an ERRNO it would be wise to return ret not fps, but this may require
changes at other places also.

re,
 wh

>  
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: wilc1000: fix incorrect copy of pmkid data

[walter harms]

Am 17.03.2017 00:21, schrieb Colin King:
> From: Colin Ian King 
> 
> The pmkid data is meant be be copied to the previous item in the
> pmkidlist, however the code is just copying the data to itself because
> the src index into pmkidlist is the same as the dst index into pmkidlist.
> Fix this with i + 1 instead of i.
> 
> Detected by CoverityScan,CID#13339465 ("Overlapping buffer in memory copy")
> 
> Signed-off-by: Colin Ian King 
> ---
>  drivers/staging/wilc1000/wilc_wfi_cfgoperations.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c 
> b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> index a37896f..4034f40 100644
> --- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> +++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> @@ -1346,7 +1346,7 @@ static int del_pmksa(struct wiphy *wiphy, struct 
> net_device *netdev,
>  priv->pmkid_list.pmkidlist[i + 1].bssid,
>  ETH_ALEN);
>   memcpy(priv->pmkid_list.pmkidlist[i].pmkid,
> -priv->pmkid_list.pmkidlist[i].pmkid,
> +priv->pmkid_list.pmkidlist[i + 1].pmkid,
>  PMKID_LEN);
>   }
>   priv->pmkid_list.numpmkid--;



perhaps we can also simplify the error handling:
that would reduce the indentlevel by one and effectivly remove the s32Error 
variable.

if (i >= priv->pmkid_list.numpmkid || priv->pmkid_list.numpmkid <= 0)
return -EINVAL;


just my 2 cents.
re,
 wh

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: atomisp: clean up return logic, remove redunant code

[walter harms]

Am 11.03.2017 20:32, schrieb Colin King:
> From: Colin Ian King 
> 
> There is no need to check if ret is non-zero, remove this
> redundant check and just return the error status from the call
> to mt9m114_write_reg_array.
> 
> Detected by CoverityScan, CID#1416577 ("Identical code for
> different branches")
> 
> Signed-off-by: Colin Ian King 
> ---
>  drivers/staging/media/atomisp/i2c/mt9m114.c | 6 +-
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/i2c/mt9m114.c 
> b/drivers/staging/media/atomisp/i2c/mt9m114.c
> index 8762124..a555aec 100644
> --- a/drivers/staging/media/atomisp/i2c/mt9m114.c
> +++ b/drivers/staging/media/atomisp/i2c/mt9m114.c
> @@ -444,12 +444,8 @@ static int mt9m114_set_suspend(struct v4l2_subdev *sd)
>  static int mt9m114_init_common(struct v4l2_subdev *sd)
>  {
>   struct i2c_client *client = v4l2_get_subdevdata(sd);
> - int ret;
>  
> - ret = mt9m114_write_reg_array(client, mt9m114_common, PRE_POLLING);
> - if (ret)
> - return ret;
> - return ret;
> + return mt9m114_write_reg_array(client, mt9m114_common, PRE_POLLING);
>  }


any use for "client" ?

re,
 wh

>  
>  static int power_ctrl(struct v4l2_subdev *sd, bool flag)
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [patch v2] staging: bcm2835-camera: fix error handling in init

[walter harms]

looks more readable now :)

Acked-by: wha...@bfs.de

Am 18.02.2017 00:20, schrieb Dan Carpenter:
> The unwinding here isn't right.  We don't free gdev[0] and instead
> free 1 step past what was allocated.  Also we can't allocate "dev" then
> we should unwind instead of returning directly.
> 
> Fixes: 7b3ad5abf027 ("staging: Import the BCM2835 MMAL-based V4L2 camera 
> driver.")
> Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
> ---
> v2: Change the style to make Walter Harms happy.  Fix some additional
> bugs I missed in the first patch.
> 
> diff --git a/drivers/staging/media/platform/bcm2835/bcm2835-camera.c 
> b/drivers/staging/media/platform/bcm2835/bcm2835-camera.c
> index ca15a698e018..c4dad30dd133 100644
> --- a/drivers/staging/media/platform/bcm2835/bcm2835-camera.c
> +++ b/drivers/staging/media/platform/bcm2835/bcm2835-camera.c
> @@ -1901,6 +1901,7 @@ static int __init bm2835_mmal_init(void)
>   unsigned int num_cameras;
>   struct vchiq_mmal_instance *instance;
>   unsigned int resolutions[MAX_BCM2835_CAMERAS][2];
> + int i;
>  
>   ret = vchiq_mmal_init();
>   if (ret < 0)
> @@ -1914,8 +1915,10 @@ static int __init bm2835_mmal_init(void)
>  
>   for (camera = 0; camera < num_cameras; camera++) {
>   dev = kzalloc(sizeof(struct bm2835_mmal_dev), GFP_KERNEL);
> - if (!dev)
> - return -ENOMEM;
> + if (!dev) {
> + ret = -ENOMEM;
> + goto cleanup_gdev;
> + }
>  
>   dev->camera_num = camera;
>   dev->max_width = resolutions[camera][0];
> @@ -1998,9 +2001,10 @@ static int __init bm2835_mmal_init(void)
>  free_dev:
>   kfree(dev);
>  
> - for ( ; camera > 0; camera--) {
> - bcm2835_cleanup_instance(gdev[camera]);
> - gdev[camera] = NULL;
> +cleanup_gdev:
> + for (i = 0; i < camera; i++) {
> + bcm2835_cleanup_instance(gdev[i]);
> + gdev[i] = NULL;
>   }
>   pr_info("%s: error %d while loading driver\n",
>   BM2835_MMAL_MODULE_NAME, ret);
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [patch] staging: bcm2835-camera: free first element in array

[walter harms]

Am 15.02.2017 13:25, schrieb Dan Carpenter:
> We should free gdev[0] so the > should be >=.
> 
> Fixes: 7b3ad5abf027 ("staging: Import the BCM2835 MMAL-based V4L2 camera 
> driver.")
> Signed-off-by: Dan Carpenter 
> 
> diff --git a/drivers/staging/media/platform/bcm2835/bcm2835-camera.c 
> b/drivers/staging/media/platform/bcm2835/bcm2835-camera.c
> index ca15a698e018..9bcd8e546a14 100644
> --- a/drivers/staging/media/platform/bcm2835/bcm2835-camera.c
> +++ b/drivers/staging/media/platform/bcm2835/bcm2835-camera.c
> @@ -1998,7 +1998,7 @@ static int __init bm2835_mmal_init(void)
>  free_dev:
>   kfree(dev);
>  
> - for ( ; camera > 0; camera--) {
> + for ( ; camera >= 0; camera--) {
>   bcm2835_cleanup_instance(gdev[camera]);
>   gdev[camera] = NULL;
>   }

since we already know that programmers are bad in counting backwards ...

is is possible to change that into std. loop like:

 for(i=0, i< camera; i++ {
bcm2835_cleanup_instance(gdev[i]);
gdev[i] = NULL;
}

this is way a much more common pattern.


just my 2 cents,
re,
 wh

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [patch] staging: wilc1000: NULL dereference on error

[walter harms]

Am 16.07.2016 12:07, schrieb Dan Carpenter:
> We can't pass NULL pointers to destroy_workqueue().
> 
> Signed-off-by: Dan Carpenter 
> 
> diff --git a/drivers/staging/wilc1000/host_interface.c 
> b/drivers/staging/wilc1000/host_interface.c
> index 0b1760c..78f524f 100644
> --- a/drivers/staging/wilc1000/host_interface.c
> +++ b/drivers/staging/wilc1000/host_interface.c
> @@ -3363,7 +3363,7 @@ int wilc_init(struct net_device *dev, struct 
> host_if_drv **hif_drv_handler)
>   if (!hif_workqueue) {
>   netdev_err(vif->ndev, "Failed to create workqueue\n");
>   result = -ENOMEM;
> - goto _fail_mq_;
> + goto _fail_;
>   }
>  


maybe this is the point where return -ENOMEM; is the most simple solution ?

re,
 wh


>   setup_timer(_rssi, GetPeriodicRSSI,
> @@ -3391,7 +3391,6 @@ int wilc_init(struct net_device *dev, struct 
> host_if_drv **hif_drv_handler)
>  
>   clients_count++;
>  
> -_fail_mq_:
>   destroy_workqueue(hif_workqueue);
>  _fail_:
>   return result;
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [patch] staging: comedi: dt2811: fix a precedence bug

[walter harms]

Am 21.06.2016 15:56, schrieb Ian Abbott:
> On 21/06/16 12:46, Dan Carpenter wrote:
>> Bitwise | has higher precedence than ?: so we need to add some
>> parenthesis for this to work as intended.
>>
>> Fixes: 7c9574090d30 ('staging: comedi: dt2811: simplify A/D reference
>> configuration')
>> Signed-off-by: Dan Carpenter 
>>
>> diff --git a/drivers/staging/comedi/drivers/dt2811.c
>> b/drivers/staging/comedi/drivers/dt2811.c
>> index 904f6377..8bbd938 100644
>> --- a/drivers/staging/comedi/drivers/dt2811.c
>> +++ b/drivers/staging/comedi/drivers/dt2811.c
>> @@ -588,8 +588,8 @@ static int dt2811_attach(struct comedi_device
>> *dev, struct comedi_devconfig *it)
>>  s = >subdevices[0];
>>  s->type= COMEDI_SUBD_AI;
>>  s->subdev_flags= SDF_READABLE |
>> -  (it->options[2] == 1) ? SDF_DIFF :
>> -  (it->options[2] == 2) ? SDF_COMMON : SDF_GROUND;
>> +  ((it->options[2] == 1) ? SDF_DIFF :
>> +   (it->options[2] == 2) ? SDF_COMMON : SDF_GROUND);
>>  s->n_chan= (it->options[2] == 1) ? 8 : 16;
>>  s->maxdata= 0x0fff;
>>  s->range_table= board->is_pgh ? _pgh_ai_ranges
>>
> 

perhaps we can improve readability with something like that (untested):

switch(it->options[2]) {
case 1:
 s->subdev_flags= SDF_READABLE|SDF_DIFF;
 break;
case 2:
 s->subdev_flags= SDF_READABLE|SDF_COMMON;
 break;
default:
 s->subdev_flags=SDF_READABLE|SDF_GROUND;
}

so everyone has a chance to see what is going on.

just my 2 cents,

re,
 wh


> Nice catch, thanks!
> 
> Reviewed-by: Ian Abbott 
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: dgnc: replace dgnc_offset_table with bit shift.

[walter harms]

Am 25.03.2016 12:33, schrieb Daeseok Youn:
> the dgnc_offset_table has a same value with (1 << port).
> So I tried to replace dgnc_offset_table array with 1 << port.
> And also there are redundant assignments(tmp and current_port)
> inside while loop for checking uart port, and remove them.
> 
> Signed-off-by: Daeseok Youn 
> ---
> Greg, This patch depends on previous patches.
> here are links(previous):
> 1. https://lkml.org/lkml/2016/3/24/661
> 2. https://lkml.org/lkml/2016/3/24/663
> 
> if those patches are failed to merge, I will send them again after
> fixing them.
> 
> thanks.
> 
>  drivers/staging/dgnc/dgnc_neo.c | 44 
> +++--
>  1 file changed, 20 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/staging/dgnc/dgnc_neo.c b/drivers/staging/dgnc/dgnc_neo.c
> index d732e6e..8b6bc73 100644
> --- a/drivers/staging/dgnc/dgnc_neo.c
> +++ b/drivers/staging/dgnc/dgnc_neo.c
> @@ -77,9 +77,6 @@ struct board_ops dgnc_neo_ops = {
>   .send_immediate_char =  neo_send_immediate_char
>  };
>  
> -static uint dgnc_offset_table[8] = { 0x01, 0x02, 0x04, 0x08,
> -  0x10, 0x20, 0x40, 0x80 };
> -
>  /*
>   * This function allows calls to ensure that all outstanding
>   * PCI writes have been completed, by doing a PCI read against
> @@ -923,9 +920,7 @@ static irqreturn_t neo_intr(int irq, void *voidbrd)
>   struct dgnc_board *brd = voidbrd;
>   struct channel_t *ch;
>   int port = 0;
> - int type = 0;
> - int current_port;
> - u32 tmp;
> + int type;
>   u32 uart_poll;
>   unsigned long flags;
>   unsigned long flags2;
> @@ -960,28 +955,29 @@ static irqreturn_t neo_intr(int irq, void *voidbrd)
>  
>   /* At this point, we have at least SOMETHING to service, dig further... 
> */
>  
> - current_port = 0;
> -
>   /* Loop on each port */
>   while ((uart_poll & 0xff) != 0) {
> - tmp = uart_poll;
> -
> - /* Check current port to see if it has interrupt pending */
> - if ((tmp & dgnc_offset_table[current_port]) != 0) {
> - port = current_port;
> - type = tmp >> (8 + (port * 3));
> - type &= 0x7;
> - } else {
> - current_port++;
> - continue;
> - }
> + int i;
>  
> - /* Remove this port + type from uart_poll */
> - uart_poll &= ~(dgnc_offset_table[port]);
> + type = 0;
>  
> - if (!type) {
> - /* If no type, just ignore it, and move onto next port 
> */
> - continue;
> + for (i = port; i < MAXPORTS; i++) {
> + unsigned int offset_table = 0x1 << i;
> +
> + /* Check current port to see
> +  * if it has interrupt pending
> +  */
> + if ((uart_poll & offset_table) != 0) {
> + port = i;
> + type = uart_poll >> (8 + (port * 3));
> + type &= 0x7;
> +
> + /* Remove this port + type from uart_poll */
> + uart_poll &= ~(offset_table);

why not: art_poll &= ~ (0x1 << i);
then you can easy eliminate offset_table

btw: why do you need i ?

re,
 wh

> + }
> +
> + if (type)
> + break;
>   }
>  
>   /* Switch on type of interrupt we have */
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: dgnc: fix camelcase of SerialDriver and PrintDriver

[walter harms]

You have send this patch before, right ?
then it is a good custom to have something like: [Patch V2] in the
subject line. In the comment you should write somethink like

v2:  fix withspace damage
v1:  fix issue

Otherwise none of the reviewer maintainer will see what was changes.
Sometimes patch run a few rounds before applied.

just my two cents

re,
 wh

Am 22.03.2016 10:20, schrieb Daeseok Youn:
> fix the checkpatch.pl warning about CamelCase.
> 
> Signed-off-by: Daeseok Youn 
> ---
>  drivers/staging/dgnc/dgnc_driver.h |   4 +-
>  drivers/staging/dgnc/dgnc_tty.c| 118 
> ++---
>  2 files changed, 61 insertions(+), 61 deletions(-)
> 
> diff --git a/drivers/staging/dgnc/dgnc_driver.h 
> b/drivers/staging/dgnc/dgnc_driver.h
> index e4be81b..953c891 100644
> --- a/drivers/staging/dgnc/dgnc_driver.h
> +++ b/drivers/staging/dgnc/dgnc_driver.h
> @@ -202,9 +202,9 @@ struct dgnc_board {
>* to our channels.
>*/
>  
> - struct tty_driver   SerialDriver;
> + struct tty_driver serial_driver;
>   charSerialName[200];
> - struct tty_driver   PrintDriver;
> + struct tty_driver print_driver;
>   charPrintName[200];
>  
>   booldgnc_Major_Serial_Registered;
> diff --git a/drivers/staging/dgnc/dgnc_tty.c b/drivers/staging/dgnc/dgnc_tty.c
> index bcd2bdf..081ac75 100644
> --- a/drivers/staging/dgnc/dgnc_tty.c
> +++ b/drivers/staging/dgnc/dgnc_tty.c
> @@ -178,20 +178,20 @@ int dgnc_tty_register(struct dgnc_board *brd)
>  {
>   int rc = 0;
>  
> - brd->SerialDriver.magic = TTY_DRIVER_MAGIC;
> + brd->serial_driver.magic = TTY_DRIVER_MAGIC;
>  
>   snprintf(brd->SerialName, MAXTTYNAMELEN, "tty_dgnc_%d_", brd->boardnum);
>  
> - brd->SerialDriver.name = brd->SerialName;
> - brd->SerialDriver.name_base = 0;
> - brd->SerialDriver.major = 0;
> - brd->SerialDriver.minor_start = 0;
> - brd->SerialDriver.num = brd->maxports;
> - brd->SerialDriver.type = TTY_DRIVER_TYPE_SERIAL;
> - brd->SerialDriver.subtype = SERIAL_TYPE_NORMAL;
> - brd->SerialDriver.init_termios = DgncDefaultTermios;
> - brd->SerialDriver.driver_name = DRVSTR;
> - brd->SerialDriver.flags = (TTY_DRIVER_REAL_RAW |
> + brd->serial_driver.name = brd->SerialName;
> + brd->serial_driver.name_base = 0;
> + brd->serial_driver.major = 0;
> + brd->serial_driver.minor_start = 0;
> + brd->serial_driver.num = brd->maxports;
> + brd->serial_driver.type = TTY_DRIVER_TYPE_SERIAL;
> + brd->serial_driver.subtype = SERIAL_TYPE_NORMAL;
> + brd->serial_driver.init_termios = DgncDefaultTermios;
> + brd->serial_driver.driver_name = DRVSTR;
> + brd->serial_driver.flags = (TTY_DRIVER_REAL_RAW |
>  TTY_DRIVER_DYNAMIC_DEV |
>  TTY_DRIVER_HARDWARE_BREAK);
>  
> @@ -199,28 +199,28 @@ int dgnc_tty_register(struct dgnc_board *brd)
>* The kernel wants space to store pointers to
>* tty_struct's and termios's.
>*/
> - brd->SerialDriver.ttys = kcalloc(brd->maxports,
> -  sizeof(*brd->SerialDriver.ttys),
> + brd->serial_driver.ttys = kcalloc(brd->maxports,
> +  sizeof(*brd->serial_driver.ttys),
>GFP_KERNEL);
> - if (!brd->SerialDriver.ttys)
> + if (!brd->serial_driver.ttys)
>   return -ENOMEM;
>  
> - kref_init(>SerialDriver.kref);
> - brd->SerialDriver.termios = kcalloc(brd->maxports,
> - sizeof(*brd->SerialDriver.termios),
> + kref_init(>serial_driver.kref);
> + brd->serial_driver.termios = kcalloc(brd->maxports,
> + sizeof(*brd->serial_driver.termios),
>   GFP_KERNEL);
> - if (!brd->SerialDriver.termios)
> + if (!brd->serial_driver.termios)
>   return -ENOMEM;
>  
>   /*
>* Entry points for driver.  Called by the kernel from
>* tty_io.c and n_tty.c.
>*/
> - tty_set_operations(>SerialDriver, _tty_ops);
> + tty_set_operations(>serial_driver, _tty_ops);
>  
>   if (!brd->dgnc_Major_Serial_Registered) {
>   /* Register tty devices */
> - rc = tty_register_driver(>SerialDriver);
> + rc = tty_register_driver(>serial_driver);
>   if (rc < 0) {
>   dev_dbg(>pdev->dev,
>   "Can't register tty device (%d)\n", rc);
> @@ -234,19 +234,19 @@ int dgnc_tty_register(struct dgnc_board *brd)
>* again, separately so we don't get the LD confused about what major
>* we are when we get into the dgnc_tty_open() routine.
>*/
> - brd->PrintDriver.magic = 

Re: [patch] staging: wilc1000: fix mgmt_tx()

[walter harms]

Am 10.02.2016 10:05, schrieb Dan Carpenter:
> There was a missing curly brace so this function returns failure instead
> of succeeding.
> 
> Fixes: 06fb9336acdc ('staging: wilc1000: wilc_wfi_cfgoperations.c: replaces 
> PRINT_ER with netdev_err')
> Signed-off-by: Dan Carpenter 
> 
> diff --git a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c 
> b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> index bf264d3..97d1b80 100644
> --- a/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> +++ b/drivers/staging/wilc1000/wilc_wfi_cfgoperations.c
> @@ -1832,9 +1832,10 @@ static int mgmt_tx(struct wiphy *wiphy,
>   return -EFAULT;
>  
>   mgmt_tx->buff = kmalloc(buf_len, GFP_KERNEL);
> - if (!mgmt_tx->buff)
> + if (!mgmt_tx->buff) {
>   kfree(mgmt_tx);
> - return -EFAULT;
> + return -ENOMEM;
> + }
>  
>   memcpy(mgmt_tx->buff, buf, len);
>   mgmt_tx->size = len;


perhaps this is a case for kmemdup() ?

mgmt_tx->buff = kmemdup(buf, buf_len or len ?, GFP_KERNEL);

sorry, i can not see what len from this patch.

re,
 wh
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [patch] staging: comedi: das16: remove a duplicate condition

[walter harms]

Am 29.07.2015 23:36, schrieb Dan Carpenter:
 We checked that it-options[3] was non-zero on the line before so
 there is no need to check again.
 
 Signed-off-by: Dan Carpenter dan.carpen...@oracle.com
 
 diff --git a/drivers/staging/comedi/drivers/das16.c 
 b/drivers/staging/comedi/drivers/das16.c
 index d7cf4b1..056bca9 100644
 --- a/drivers/staging/comedi/drivers/das16.c
 +++ b/drivers/staging/comedi/drivers/das16.c
 @@ -1032,8 +1032,7 @@ static int das16_attach(struct comedi_device *dev, 
 struct comedi_devconfig *it)
  
   /*  check that clock setting is valid */
   if (it-options[3]) {
 - if (it-options[3] != 0 
 - it-options[3] != 1  it-options[3] != 10) {
 + if (it-options[3] != 1  it-options[3] != 10) {
   dev_err(dev-class_dev,
   Invalid option. Master clock must be set to 1 
 or 10 (MHz)\n);
   return -EINVAL;

mmh, acording to the error messages 1 or 10 is allowed, obviously 0 is/was also 
valid.
I would suggest to put that into one if () to make things more obvious.
It 0 is also valid a minor tweak to the error msg would be nice.


re,
 wh

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 09/12] staging: lustre: obdclass: Use !x to check for kzalloc failure

[walter harms]

Am 20.06.2015 18:59, schrieb Julia Lawall:
 !x is more normal for kzalloc failure in the kernel.
 
 The semantic patch that makes this change is as follows:
 (http://coccinelle.lip6.fr/)
 
 // smpl
 @@
 expression x;
 statement S1, S2;
 @@
 
 x = kzalloc(...);
 if (
 - x == NULL
 + !x
  ) S1 else S2
 // /smpl
 
 Signed-off-by: Julia Lawall julia.law...@lip6.fr
 
 ---
  drivers/staging/lustre/lustre/obdclass/class_obd.c  |2 +-
  drivers/staging/lustre/lustre/obdclass/genops.c |6 +++---
  drivers/staging/lustre/lustre/obdclass/llog.c   |6 +++---
  drivers/staging/lustre/lustre/obdclass/lprocfs_status.c |2 +-
  drivers/staging/lustre/lustre/obdclass/lustre_peer.c|2 +-
  drivers/staging/lustre/lustre/obdclass/obd_config.c |   10 +-
  drivers/staging/lustre/lustre/obdclass/obd_mount.c  |   12 ++--
  7 files changed, 20 insertions(+), 20 deletions(-)
 
 diff -u -p a/drivers/staging/lustre/lustre/obdclass/obd_mount.c 
 b/drivers/staging/lustre/lustre/obdclass/obd_mount.c
 --- a/drivers/staging/lustre/lustre/obdclass/obd_mount.c
 +++ b/drivers/staging/lustre/lustre/obdclass/obd_mount.c
 @@ -85,7 +85,7 @@ int lustre_process_log(struct super_bloc
   LASSERT(cfg);
  
   bufs = kzalloc(sizeof(*bufs), GFP_NOFS);
 - if (bufs == NULL)
 + if (!bufs)
   return -ENOMEM;
  
   /* mgc_process_config */
 @@ -258,7 +258,7 @@ int lustre_start_mgc(struct super_block
   mgssec = lsi-lsi_lmd-lmd_mgssec ? lsi-lsi_lmd-lmd_mgssec : ;
  
   data = kzalloc(sizeof(*data), GFP_NOFS);
 - if (data == NULL) {
 + if (!data) {
   rc = -ENOMEM;
   goto out_free;
   }
 @@ -885,7 +885,7 @@ static int lmd_parse_mgssec(struct lustr
   length = tail - ptr;
  
   lmd-lmd_mgssec = kzalloc(length + 1, GFP_NOFS);
 - if (lmd-lmd_mgssec == NULL)
 + if (!lmd-lmd_mgssec)
   return -ENOMEM;
  
   memcpy(lmd-lmd_mgssec, ptr, length);
looks like memdup()

 @@ -911,7 +911,7 @@ static int lmd_parse_string(char **handl
   length = tail - ptr;
  
   *handle = kzalloc(length + 1, GFP_NOFS);
 - if (*handle == NULL)
 + if (!*handle)
   return -ENOMEM;
  
   memcpy(*handle, ptr, length);

looks like memdup()


 @@ -941,7 +941,7 @@ static int lmd_parse_mgs(struct lustre_m
   oldlen = strlen(lmd-lmd_mgs) + 1;
  
   mgsnid = kzalloc(oldlen + length + 1, GFP_NOFS);
 - if (mgsnid == NULL)
 + if (!mgsnid)
   return -ENOMEM;
  
   if (lmd-lmd_mgs != NULL) {
 @@ -983,7 +983,7 @@ static int lmd_parse(char *options, stru
   lmd-lmd_magic = LMD_MAGIC;
  
   lmd-lmd_params = kzalloc(4096, GFP_NOFS);
 - if (lmd-lmd_params == NULL)
 + if (!lmd-lmd_params)
   return -ENOMEM;
   lmd-lmd_params[0] = '\0';
  
 diff -u -p a/drivers/staging/lustre/lustre/obdclass/obd_config.c 
 b/drivers/staging/lustre/lustre/obdclass/obd_config.c
 --- a/drivers/staging/lustre/lustre/obdclass/obd_config.c
 +++ b/drivers/staging/lustre/lustre/obdclass/obd_config.c
 @@ -835,7 +835,7 @@ int class_add_profile(int proflen, char
   CDEBUG(D_CONFIG, Add profile %s\n, prof);
  
   lprof = kzalloc(sizeof(*lprof), GFP_NOFS);
 - if (lprof == NULL)
 + if (!lprof)
   return -ENOMEM;
   INIT_LIST_HEAD(lprof-lp_list);
  
 @@ -979,7 +979,7 @@ struct lustre_cfg *lustre_cfg_rename(str
   new_len = LUSTRE_CFG_BUFLEN(cfg, 1) + strlen(new_name) - name_len;
  
   new_param = kzalloc(new_len, GFP_NOFS);
 - if (new_param == NULL)
 + if (!new_param)
   return ERR_PTR(-ENOMEM);
  
   strcpy(new_param, new_name);
 @@ -987,7 +987,7 @@ struct lustre_cfg *lustre_cfg_rename(str
   strcat(new_param, value);
  
   bufs = kzalloc(sizeof(*bufs), GFP_NOFS);
 - if (bufs == NULL) {
 + if (!bufs) {
   kfree(new_param);
   return ERR_PTR(-ENOMEM);
   }
 @@ -1461,7 +1461,7 @@ int class_config_llog_handler(const stru
   inst_len = LUSTRE_CFG_BUFLEN(lcfg, 0) +
  sizeof(clli-cfg_instance) * 2 + 4;
   inst_name = kzalloc(inst_len, GFP_NOFS);
 - if (inst_name == NULL) {
 + if (!inst_name) {
   rc = -ENOMEM;
   goto out;
   }
 @@ -1639,7 +1639,7 @@ int class_config_dump_handler(const stru
   int  rc = 0;
  
   outstr = kzalloc(256, GFP_NOFS);
 - if (outstr == NULL)
 + if (!outstr)
   return -ENOMEM;
  
   if (rec-lrh_type == OBD_CFG_REC) {
 diff -u -p a/drivers/staging/lustre/lustre/obdclass/lustre_peer.c 
 b/drivers/staging/lustre/lustre/obdclass/lustre_peer.c
 --- a/drivers/staging/lustre/lustre/obdclass/lustre_peer.c
 +++ b/drivers/staging/lustre/lustre/obdclass/lustre_peer.c
 @@ -105,7 +105,7 @@ int class_add_uuid(const 

Re: [PATCH 8/11] staging: lustre: obdclass: Use kzalloc and kfree

[walter harms]

hi Julia,
your patch seems fine.
I tried to understand the code and it seems that much of it
can be simplified by using already available functions.
I have added some comments but i am not sure what to make of it.

re,
 wh


Am 01.05.2015 17:51, schrieb Julia Lawall:
 From: Julia Lawall julia.law...@lip6.fr
 
 Replace OBD_ALLOC, OBD_ALLOC_WAIT, OBD_ALLOC_PTR, and OBD_ALLOC_PTR_WAIT by
 kalloc/kcalloc, and OBD_FREE and OBD_FREE_PTR by kfree.
 
 A simplified version of the semantic patch that makes these changes is as
 follows: (http://coccinelle.lip6.fr/)
 
 // smpl
 @@ expression ptr,size; @@
 - OBD_ALLOC(ptr,size)
 + ptr = kzalloc(size, GFP_NOFS)
 
 @@ expression ptr, size; @@
 - OBD_FREE(ptr, size);
 + kfree(ptr);
 // /smpl
 
 Signed-off-by: Julia Lawall julia.law...@lip6.fr
 
 ---
  drivers/staging/lustre/lustre/obdclass/acl.c|   29 ++---
  drivers/staging/lustre/lustre/obdclass/capa.c   |4 
  drivers/staging/lustre/lustre/obdclass/cl_io.c  |   13 +-
  drivers/staging/lustre/lustre/obdclass/cl_page.c|2 
  drivers/staging/lustre/lustre/obdclass/class_obd.c  |4 
  drivers/staging/lustre/lustre/obdclass/genops.c |   40 +++
  drivers/staging/lustre/lustre/obdclass/llog.c   |   24 ++--
  drivers/staging/lustre/lustre/obdclass/llog_obd.c   |4 
  drivers/staging/lustre/lustre/obdclass/lprocfs_status.c |   14 +-
  drivers/staging/lustre/lustre/obdclass/lu_object.c  |6 -
  drivers/staging/lustre/lustre/obdclass/lustre_handles.c |2 
  drivers/staging/lustre/lustre/obdclass/lustre_peer.c|6 -
  drivers/staging/lustre/lustre/obdclass/obd_config.c |   50 -
  drivers/staging/lustre/lustre/obdclass/obd_mount.c  |   86 
 +++-
  14 files changed, 138 insertions(+), 146 deletions(-)
 
 diff -u -p a/drivers/staging/lustre/lustre/obdclass/obd_mount.c 
 b/drivers/staging/lustre/lustre/obdclass/obd_mount.c
 --- a/drivers/staging/lustre/lustre/obdclass/obd_mount.c
 +++ b/drivers/staging/lustre/lustre/obdclass/obd_mount.c
 @@ -84,7 +84,7 @@ int lustre_process_log(struct super_bloc
   LASSERT(mgc);
   LASSERT(cfg);
  
 - OBD_ALLOC_PTR(bufs);
 + bufs = kzalloc(sizeof(*bufs), GFP_NOFS);
   if (bufs == NULL)
   return -ENOMEM;
  
 @@ -97,7 +97,7 @@ int lustre_process_log(struct super_bloc
   rc = obd_process_config(mgc, sizeof(*lcfg), lcfg);
   lustre_cfg_free(lcfg);
  
 - OBD_FREE_PTR(bufs);
 + kfree(bufs);
  
   if (rc == -EINVAL)
   LCONSOLE_ERROR_MSG(0x15b, %s: The configuration from log '%s' 
 failed from the MGS (%d).  Make sure this client and the MGS are running 
 compatible versions of Lustre.\n,
 @@ -247,8 +247,8 @@ int lustre_start_mgc(struct super_block
   mutex_lock(mgc_start_lock);
  
   len = strlen(LUSTRE_MGC_OBDNAME) + strlen(libcfs_nid2str(nid)) + 1;
 - OBD_ALLOC(mgcname, len);
 - OBD_ALLOC(niduuid, len + 2);
 + mgcname = kzalloc(len, GFP_NOFS);
 + niduuid = kzalloc(len + 2, GFP_NOFS);
   if (!mgcname || !niduuid) {
   rc = -ENOMEM;
   goto out_free;

 this can be simplified by using
   kasprintf(mgcname,%s%s, LUSTRE_MGC_OBDNAME, libcfs_nid2str(nid));

 is guess the some is true for niduuid


 @@ -257,7 +257,7 @@ int lustre_start_mgc(struct super_block
  
   mgssec = lsi-lsi_lmd-lmd_mgssec ? lsi-lsi_lmd-lmd_mgssec : ;
  
 - OBD_ALLOC_PTR(data);
 + data = kzalloc(sizeof(*data), GFP_NOFS);
   if (data == NULL) {
   rc = -ENOMEM;
   goto out_free;
 @@ -375,7 +375,7 @@ int lustre_start_mgc(struct super_block
   lsi-lsi_lmd-lmd_mgs_failnodes = 1;
  
   /* Random uuid for MGC allows easier reconnects */
 - OBD_ALLOC_PTR(uuid);
 + uuid = kzalloc(sizeof(*uuid), GFP_NOFS);
   if (!uuid) {
   rc = -ENOMEM;
   goto out_free;
 @@ -388,7 +388,7 @@ int lustre_start_mgc(struct super_block
   rc = lustre_start_simple(mgcname, LUSTRE_MGC_NAME,
(char *)uuid-uuid, LUSTRE_MGS_OBDNAME,
niduuid, NULL, NULL);
 - OBD_FREE_PTR(uuid);
 + kfree(uuid);
   if (rc)
   goto out_free;
  
 @@ -465,11 +465,11 @@ out_free:
   mutex_unlock(mgc_start_lock);
  
   if (data)
 - OBD_FREE_PTR(data);
 + kfree(data);
   if (mgcname)
 - OBD_FREE(mgcname, len);
 + kfree(mgcname);
   if (niduuid)
 - OBD_FREE(niduuid, len + 2);
 + kfree(niduuid);
   return rc;
  }
  
 @@ -513,7 +513,7 @@ static int lustre_stop_mgc(struct super_
   /* Save the obdname for cleaning the nid uuids, which are
  obdname_XX */
   len = strlen(obd-obd_name) + 6;
 - OBD_ALLOC(niduuid, len);
 + niduuid = kzalloc(len, GFP_NOFS);
   if (niduuid) {
   strcpy(niduuid, obd-obd_name);
   ptr = niduuid + strlen(niduuid);

i guess 

Re: [PATCH 2/5] staging: lustre: lnet: socklnd: Clean up memset(...)

[walter harms]

Am 18.05.2014 19:27, schrieb Joe Perches:
 On Sun, 2014-05-18 at 18:19 +0100, Masaru Nomura wrote:
 Remove prohibited space and fix line over 80 characters of
 memset(...) to meet kernel coding style.
 []
 diff --git a/drivers/staging/lustre/lnet/klnds/socklnd/socklnd.c 
 b/drivers/staging/lustre/lnet/klnds/socklnd/socklnd.c
 []
 @@ -113,7 +113,7 @@ ksocknal_create_peer(ksock_peer_t **peerp, lnet_ni_t 
 *ni, lnet_process_id_t id)
  if (peer == NULL)
  return -ENOMEM;
  
 -memset (peer, 0, sizeof (*peer));   /* NULL pointers/clear flags 
 etc */
 +memset(peer, 0, sizeof(*peer)); /* NULL pointers/clear flags etc */
 
 It looks like this memset is unnecessary
 as it's already zeroed by LIBCFS_ALLOC-
 LIBCFS_ALLOC_GFP-LIBCFS_ALLOC_POST-memset.
 
 It seems as if all these ALLOC macros could
 use quite a bit of cleaning/sorting out.
 

for a start,
some kind soul could replace the malloc()/memset() with zalloc().

re,
 wh





___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/8] staging: r8712u: Remove useless return variables

[walter harms]

Am 20.05.2014 12:33, schrieb Peter Senna Tschudin:
 This patch remove variables that are initialized with a constant,
 are never updated, and are only used as parameter of return.
 Return the constant instead of using a variable.
 
 Verified by compilation only.
 
 The coccinelle script that find and fixes this issue is:
 // smpl
 @@
 type T;
 constant C;
 identifier ret;
 @@
 - T ret = C;
 ... when != ret
 - return ret;
 + return C;
 // /smpl
 
 Signed-off-by: Peter Senna Tschudin peter.se...@gmail.com
 
 ---
  drivers/staging/rtl8712/ieee80211.c|8 ++--
  drivers/staging/rtl8712/rtl8712_cmd.c  |3 -
  drivers/staging/rtl8712/rtl8712_recv.c |7 +---
  drivers/staging/rtl8712/rtl871x_mlme.c |3 -
  drivers/staging/rtl8712/rtl871x_mp.c   |3 -
  drivers/staging/rtl8712/rtl871x_mp_ioctl.c |   49 
 +
  drivers/staging/rtl8712/rtl871x_recv.c |4 --
  7 files changed, 26 insertions(+), 51 deletions(-)
 
 diff --git a/drivers/staging/rtl8712/ieee80211.c 
 b/drivers/staging/rtl8712/ieee80211.c
 index 57fef70..fe9459e 100644
 --- a/drivers/staging/rtl8712/ieee80211.c
 +++ b/drivers/staging/rtl8712/ieee80211.c
 @@ -289,7 +289,7 @@ static int r8712_get_wpa2_cipher_suite(u8 *s)
  int r8712_parse_wpa_ie(u8 *wpa_ie, int wpa_ie_len, int *group_cipher,
int *pairwise_cipher)
  {
 - int i, ret = _SUCCESS;
 + int i;
   int left, count;
   u8 *pos;
  
 @@ -324,13 +324,13 @@ int r8712_parse_wpa_ie(u8 *wpa_ie, int wpa_ie_len, int 
 *group_cipher,
   }
   } else if (left == 1)
   return _FAIL;
 - return ret;
 + return _SUCCESS;
  }
  
  int r8712_parse_wpa2_ie(u8 *rsn_ie, int rsn_ie_len, int *group_cipher,
 int *pairwise_cipher)
  {
 - int i, ret = _SUCCESS;
 + int i;
   int left, count;
   u8 *pos;
  
 @@ -364,7 +364,7 @@ int r8712_parse_wpa2_ie(u8 *rsn_ie, int rsn_ie_len, int 
 *group_cipher,
   }
   } else if (left == 1)
   return _FAIL;
 - return ret;
 + return _SUCCESS;
  }
  
  int r8712_get_sec_ie(u8 *in_ie, uint in_len, u8 *rsn_ie, u16 *rsn_len,
 diff --git a/drivers/staging/rtl8712/rtl8712_cmd.c 
 b/drivers/staging/rtl8712/rtl8712_cmd.c
 index 1a4b7a6..8ca7d7e 100644
 --- a/drivers/staging/rtl8712/rtl8712_cmd.c
 +++ b/drivers/staging/rtl8712/rtl8712_cmd.c
 @@ -290,8 +290,7 @@ static struct cmd_obj *cmd_hdl_filter(struct _adapter 
 *padapter,
  
  static u8 check_cmd_fifo(struct _adapter *padapter, uint sz)
  {
 - u8 res = _SUCCESS;
 - return res;
 + return _SUCCESS;
  }
  

Is this function still needed ?


  u8 r8712_fw_cmd(struct _adapter *pAdapter, u32 cmd)
 diff --git a/drivers/staging/rtl8712/rtl8712_recv.c 
 b/drivers/staging/rtl8712/rtl8712_recv.c
 index 0723b2f..667398a 100644
 --- a/drivers/staging/rtl8712/rtl8712_recv.c
 +++ b/drivers/staging/rtl8712/rtl8712_recv.c
 @@ -123,8 +123,6 @@ void r8712_free_recv_priv(struct recv_priv *precvpriv)
  
  int r8712_init_recvbuf(struct _adapter *padapter, struct recv_buf *precvbuf)
  {
 - int res = _SUCCESS;
 -
   precvbuf-transfer_len = 0;
   precvbuf-len = 0;
   precvbuf-ref_cnt = 0;
 @@ -134,7 +132,7 @@ int r8712_init_recvbuf(struct _adapter *padapter, struct 
 recv_buf *precvbuf)
   precvbuf-ptail = precvbuf-pbuf;
   precvbuf-pend = precvbuf-pdata + MAX_RECVBUF_SZ;
   }
 - return res;
 + return _SUCCESS;
  }
  
  int r8712_free_recvframe(union recv_frame *precvframe,
 @@ -347,7 +345,6 @@ static int amsdu_to_msdu(struct _adapter *padapter, union 
 recv_frame *prframe)
   _pkt *sub_skb, *subframes[MAX_SUBFRAME_COUNT];
   struct recv_priv *precvpriv = padapter-recvpriv;
   struct  __queue *pfree_recv_queue = (precvpriv-free_recv_queue);
 - int ret = _SUCCESS;
  
   nr_subframes = 0;
   pattrib = prframe-u.hdr.attrib;
 @@ -435,7 +432,7 @@ static int amsdu_to_msdu(struct _adapter *padapter, union 
 recv_frame *prframe)
  exit:
   prframe-u.hdr.len = 0;
   r8712_free_recvframe(prframe, pfree_recv_queue);
 - return ret;
 + return _SUCCESS;
  }
  
  void r8712_rxcmd_event_hdl(struct _adapter *padapter, void *prxcmdbuf)
 diff --git a/drivers/staging/rtl8712/rtl871x_mlme.c 
 b/drivers/staging/rtl8712/rtl871x_mlme.c
 index 3ea99ae..05d6e65 100644
 --- a/drivers/staging/rtl8712/rtl871x_mlme.c
 +++ b/drivers/staging/rtl8712/rtl871x_mlme.c
 @@ -1211,7 +1211,6 @@ sint r8712_set_auth(struct _adapter *adapter,
   struct cmd_priv *pcmdpriv = adapter-cmdpriv;
   struct cmd_obj *pcmd;
   struct setauth_parm *psetauthparm;
 - sint ret = _SUCCESS;
  
   pcmd = (struct cmd_obj *)_malloc(sizeof(struct cmd_obj));
   if (pcmd == NULL)
 @@ -1232,7 +1231,7 @@ sint r8712_set_auth(struct _adapter *adapter,
   pcmd-rspsz = 0;
   _init_listhead(pcmd-list);
   r8712_enqueue_cmd(pcmdpriv, pcmd);
 - return ret;
 + return _SUCCESS;
  }
  
  

Re: [PATCH 3/8] drivers/staging: Remove useless return variables

[walter harms]

Am 20.05.2014 12:33, schrieb Peter Senna Tschudin:
 This patch remove variables that are initialized with a constant,
 are never updated, and are only used as parameter of return.
 Return the constant instead of using a variable.
 
 Verified by compilation only.
 
 The coccinelle script that find and fixes this issue is:
 // smpl
 @@
 type T;
 constant C;
 identifier ret;
 @@
 - T ret = C;
 ... when != ret
 - return ret;
 + return C;
 // /smpl
 
 Signed-off-by: Peter Senna Tschudin peter.se...@gmail.com
 
 ---
  drivers/staging/bcm/Bcmchar.c |3 +--
  drivers/staging/bcm/InterfaceIdleMode.c   |3 +--
  drivers/staging/bcm/PHSModule.c   |9 +++--
  drivers/staging/gdm72xx/gdm_wimax.c   |3 +--
  drivers/staging/rtl8192e/rtl8192e/r8192E_phy.c|3 +--
  drivers/staging/rtl8192e/rtllib_rx.c  |3 +--
  drivers/staging/rtl8192e/rtllib_softmac.c |3 +--
  drivers/staging/rtl8192e/rtllib_softmac_wx.c  |4 +---
  drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c |3 +--
  drivers/staging/rtl8192u/ieee80211/ieee80211_softmac.c|3 +--
  drivers/staging/rtl8192u/ieee80211/ieee80211_softmac_wx.c |4 +---
  drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c   |3 +--
  drivers/staging/rtl8192u/r819xU_cmdpkt.c  |3 +--
  drivers/staging/rtl8192u/r819xU_phy.c |3 +--
  drivers/staging/sep/sep_main.c|3 +--
  drivers/staging/silicom/bpctl_mod.c   |4 ++--
  drivers/staging/wlan-ng/hfa384x_usb.c |3 +--
  drivers/staging/wlan-ng/p80211req.c   |3 +--
  18 files changed, 21 insertions(+), 42 deletions(-)
 
 diff --git a/drivers/staging/bcm/Bcmchar.c b/drivers/staging/bcm/Bcmchar.c
 index ae7490b..777a13a 100644
 --- a/drivers/staging/bcm/Bcmchar.c
 +++ b/drivers/staging/bcm/Bcmchar.c
 @@ -1800,7 +1800,6 @@ static int bcm_char_ioctl_flash2x_section_bitmap(void 
 __user *argp,
  {
   struct bcm_flash2x_bitmap *psFlash2xBitMap;
   struct bcm_ioctl_buffer IoBuffer;
 - INT Status = STATUS_FAILURE;
  
  BCM_DEBUG_PRINT(Adapter, DBG_TYPE_OTHERS, OSAL_DBG, DBG_LVL_ALL,
   IOCTL_BCM_GET_FLASH2X_SECTION_BITMAP Called);
 @@ -1841,7 +1840,7 @@ BCM_DEBUG_PRINT(Adapter, DBG_TYPE_OTHERS, OSAL_DBG, 
 DBG_LVL_ALL,
   }
  
   kfree(psFlash2xBitMap);
 - return Status;
 + return STATUS_FAILURE;
  }
  
  static int bcm_char_ioctl_set_active_section(void __user *argp,
 diff --git a/drivers/staging/bcm/InterfaceIdleMode.c 
 b/drivers/staging/bcm/InterfaceIdleMode.c
 index fecf81f..c84ee49 100644
 --- a/drivers/staging/bcm/InterfaceIdleMode.c
 +++ b/drivers/staging/bcm/InterfaceIdleMode.c
 @@ -223,7 +223,6 @@ static int InterfaceAbortIdlemode(struct bcm_mini_adapter 
 *Adapter,
  }
  int InterfaceIdleModeWakeup(struct bcm_mini_adapter *Adapter)
  {
 - ULONG   Status = 0;
   if (Adapter-bTriedToWakeUpFromlowPowerMode) {
   BCM_DEBUG_PRINT(Adapter, DBG_TYPE_OTHERS,
   IDLE_MODE, DBG_LVL_ALL,
 @@ -233,7 +232,7 @@ int InterfaceIdleModeWakeup(struct bcm_mini_adapter 
 *Adapter)
   InterfaceAbortIdlemode(Adapter, Adapter-usIdleModePattern);
  
   }
 - return Status;
 + return 0;
  }
  
  void InterfaceHandleShutdownModeWakeup(struct bcm_mini_adapter *Adapter)
 diff --git a/drivers/staging/bcm/PHSModule.c b/drivers/staging/bcm/PHSModule.c
 index afc7bcc..07c5a0b 100644
 --- a/drivers/staging/bcm/PHSModule.c
 +++ b/drivers/staging/bcm/PHSModule.c
 @@ -409,7 +409,6 @@ ULONG PhsUpdateClassifierRule(IN void *pvContext,
   */
  ULONG PhsDeletePHSRule(IN void *pvContext, IN B_UINT16 uiVcid, IN B_UINT8 
 u8PHSI)
  {
 - ULONG lStatus = 0;
   UINT nSFIndex = 0, nClsidIndex = 0;
   struct bcm_phs_entry *pstServiceFlowEntry = NULL;
   struct bcm_phs_classifier_table *pstClassifierRulesTable = NULL;
 @@ -446,7 +445,7 @@ ULONG PhsDeletePHSRule(IN void *pvContext, IN B_UINT16 
 uiVcid, IN B_UINT8 u8PHSI
   }
   }
   }
 - return lStatus;
 + return 0;
  }
  
  /*
 @@ -467,7 +466,6 @@ ULONG PhsDeletePHSRule(IN void *pvContext, IN B_UINT16 
 uiVcid, IN B_UINT8 u8PHSI
   */
  ULONG PhsDeleteClassifierRule(IN void *pvContext, IN B_UINT16 uiVcid, IN 
 B_UINT16 uiClsId)
  {
 - ULONG lStatus = 0;
   UINT nSFIndex = 0, nClsidIndex = 0;
   struct bcm_phs_entry *pstServiceFlowEntry = NULL;
   struct bcm_phs_classifier_entry *pstClassifierEntry = NULL;
 @@ -504,7 +502,7 @@ ULONG PhsDeleteClassifierRule(IN void *pvContext, IN 
 B_UINT16 uiVcid, IN B_UINT1
   memset(pstClassifierEntry, 0, sizeof(struct 
 bcm_phs_classifier_entry));
   }
   }
 - return lStatus;
 + return 0;
  }
  
  /*
 @@ -524,7 +522,6 @@ ULONG PhsDeleteClassifierRule(IN 

Re: [PATCH 1/8] staging: r8712u: Remove useless return variables

[walter harms]

Am 20.05.2014 13:41, schrieb Dan Carpenter:
 Those concerns are valid but the code was like that in the original so
 we should merge this patch as is and hope some volunteer will fix things
 up in a follow on patch.
 
 Fixing them in this patch would be a mistake anyway because of the one
 thing per patch rule.
 

I see this as a bordercase, the patch from Peter is correct in the context of
removing useless return variables. I question the whole function in the hope
that the maintainer will decide that the function can go completely.

re,
 wh
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [patch 2/2 v2] staging: lustre: integer overflow in obd_ioctl_is_invalid()

[walter harms]

Am 28.04.2014 12:58, schrieb Dan Carpenter:
 The obd_ioctl_getdata() function caps data-ioc_len at
 OBD_MAX_IOCTL_BUFFER and then calls this obd_ioctl_is_invalid() to check
 that the other values inside data are valid.
 
 There are several lengths inside data but when they are added together
 they must not be larger than data-ioc_len.  The checks against
 (data-ioc_inllen1  (130)) are supposed to ensure that the addition
 does not have an integer overflow.  But (130) * 4 actually can
 overflow 32 bits, so the checks are insufficient.
 
 I have changed it to  OBD_MAX_IOCTL_BUFFER instead.
 
 Signed-off-by: Dan Carpenter dan.carpen...@oracle.com
 ---
 v2: Updated the error messages as Walter Harms pointed out.
 
 diff --git a/drivers/staging/lustre/lustre/include/lustre_lib.h 
 b/drivers/staging/lustre/lustre/include/lustre_lib.h
 index bdc9812..3c26bbd 100644
 --- a/drivers/staging/lustre/lustre/include/lustre_lib.h
 +++ b/drivers/staging/lustre/lustre/include/lustre_lib.h
 @@ -179,24 +179,25 @@ static inline int obd_ioctl_packlen(struct 
 obd_ioctl_data *data)
  
  static inline int obd_ioctl_is_invalid(struct obd_ioctl_data *data)
  {
 - if (data-ioc_len  (130)) {
 - CERROR(OBD ioctl: ioc_len larger than 130\n);
 + if (data-ioc_len  OBD_MAX_IOCTL_BUFFER) {
 + CERROR(OBD ioctl: ioc_len larger than %d\n,
 +OBD_MAX_IOCTL_BUFFER);
   return 1;
   }
 - if (data-ioc_inllen1  (130)) {
 - CERROR(OBD ioctl: ioc_inllen1 larger than 130\n);
 + if (data-ioc_inllen1  OBD_MAX_IOCTL_BUFFER) {
 + CERROR(OBD ioctl: ioc_inllen1 larger than ioc_len\n);
   return 1;
   }

The error mention ioc_len the compare is OBD_MAX_IOCTL_BUFFER ?
Is that intentional ?

re,
 wh

 - if (data-ioc_inllen2  (130)) {
 - CERROR(OBD ioctl: ioc_inllen2 larger than 130\n);
 + if (data-ioc_inllen2  OBD_MAX_IOCTL_BUFFER) {
 + CERROR(OBD ioctl: ioc_inllen2 larger than ioc_len\n);
   return 1;
   }
 - if (data-ioc_inllen3  (130)) {
 - CERROR(OBD ioctl: ioc_inllen3 larger than 130\n);
 + if (data-ioc_inllen3  OBD_MAX_IOCTL_BUFFER) {
 + CERROR(OBD ioctl: ioc_inllen3 larger than ioc_len\n);
   return 1;
   }
 - if (data-ioc_inllen4  (130)) {
 - CERROR(OBD ioctl: ioc_inllen4 larger than 130\n);
 + if (data-ioc_inllen4  OBD_MAX_IOCTL_BUFFER) {
 + CERROR(OBD ioctl: ioc_inllen4 larger than ioc_len\n);
   return 1;
   }
   if (data-ioc_inlbuf1  !data-ioc_inllen1) {
 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [patch 2/2] staging: lustre: integer overflow in obd_ioctl_is_invalid()

[walter harms]

Am 24.04.2014 23:49, schrieb Dan Carpenter:
 The obd_ioctl_getdata() function caps data-ioc_len at
 OBD_MAX_IOCTL_BUFFER and then calls this obd_ioctl_is_invalid() to check
 that the other values inside data are valid.
 
 There are several lengths inside data but when they are added together
 they must not be larger than data-ioc_len.  The checks against
 (data-ioc_inllen1  (130)) are supposed to ensure that the addition
 does not have an integer overflow.  But (130) * 4 actually can
 overflow 32 bits so the checks are insufficient.
 
 I have changed it to  OBD_MAX_IOCTL_BUFFER instead.
 
 Signed-off-by: Dan Carpenter dan.carpen...@oracle.com
 
 diff --git a/drivers/staging/lustre/lustre/include/lustre_lib.h 
 b/drivers/staging/lustre/lustre/include/lustre_lib.h
 index 0368ca6..04f549e 100644
 --- a/drivers/staging/lustre/lustre/include/lustre_lib.h
 +++ b/drivers/staging/lustre/lustre/include/lustre_lib.h
 @@ -192,23 +192,23 @@ static inline int obd_ioctl_packlen(struct 
 obd_ioctl_data *data)
  
  static inline int obd_ioctl_is_invalid(struct obd_ioctl_data *data)
  {
 - if (data-ioc_len  (130)) {
 + if (data-ioc_len  OBD_MAX_IOCTL_BUFFER) {
   CERROR(OBD ioctl: ioc_len larger than 130\n);
   return 1;
   }

I would suggest to adjust the errormsg also like:
CERROR(OBD ioctl: ioc_len larger than OBD_MAX_IOCTL_BUFFER\n);

otherwise future debuggers will be confused.

just my 2 cents

re,
 wh



 - if (data-ioc_inllen1  (130)) {
 + if (data-ioc_inllen1  OBD_MAX_IOCTL_BUFFER) {
   CERROR(OBD ioctl: ioc_inllen1 larger than 130\n);
   return 1;
   }
 - if (data-ioc_inllen2  (130)) {
 + if (data-ioc_inllen2  OBD_MAX_IOCTL_BUFFER) {
   CERROR(OBD ioctl: ioc_inllen2 larger than 130\n);
   return 1;
   }
 - if (data-ioc_inllen3  (130)) {
 + if (data-ioc_inllen3  OBD_MAX_IOCTL_BUFFER) {
   CERROR(OBD ioctl: ioc_inllen3 larger than 130\n);
   return 1;
   }
 - if (data-ioc_inllen4  (130)) {
 + if (data-ioc_inllen4  OBD_MAX_IOCTL_BUFFER) {
   CERROR(OBD ioctl: ioc_inllen4 larger than 130\n);
   return 1;
   }
 --
 To unsubscribe from this list: send the line unsubscribe kernel-janitors in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [patch 2/2] staging: r8188eu: overflow in rtw_p2p_get_go_device_address()

[walter harms]

Am 03.02.2014 23:38, schrieb Dan Carpenter:
 The go_devadd_str[] array is two characters too small to hold the
 address so we corrupt memory.
 
 I've changed the user space API slightly and I don't have a way to test
 if this breaks anything.  In the original code we truncated away the
 last digit of the address and the NUL terminator so it was already a bit
 broken.
 
 Signed-off-by: Dan Carpenter dan.carpen...@oracle.com
 
 diff --git a/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c 
 b/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c
 index dec992569476..4ad80ae1067f 100644
 --- a/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c
 +++ b/drivers/staging/rtl8188eu/os_dep/ioctl_linux.c
 @@ -3164,9 +3164,7 @@ static int rtw_p2p_get_go_device_address(struct 
 net_device *dev,
   u8 *p2pie;
   uint p2pielen = 0, attr_contentlen = 0;
   u8 attr_content[100] = {0x00};
 -
 - u8 go_devadd_str[17 + 10] = {0x00};
 - /*  +10 is for the str go_devadd =, we have to clear it at 
 wrqu-data.pointer */
 + u8 go_devadd_str[17 + 12] = {};


you are deleting the explanation for the magic numbers here,
- intentionally  ?

NTL, it would be nice to have a full explanation like
 10= space for  go_devadd =
 17= space for attr_content %.2X:%.2X:%.2X:%.2X:%.2X:%.2X
re,
 wh

   /*  Commented by Albert 20121209 */
   /*  The input data is the GO's interface address which the 
 application wants to know its device address. */
 @@ -3223,12 +3221,12 @@ static int rtw_p2p_get_go_device_address(struct 
 net_device *dev,
   spin_unlock_bh(pmlmepriv-scanned_queue.lock);
  
   if (!blnMatch)
 - sprintf(go_devadd_str, \n\ndev_add = NULL);
 + snprintf(go_devadd_str, sizeof(go_devadd_str), \n\ndev_add = 
 NULL);
   else
 - sprintf(go_devadd_str, \n\ndev_add 
 =%.2X:%.2X:%.2X:%.2X:%.2X:%.2X,
 + snprintf(go_devadd_str, sizeof(go_devadd_str), \n\ndev_add 
 =%.2X:%.2X:%.2X:%.2X:%.2X:%.2X,
   attr_content[0], attr_content[1], attr_content[2], 
 attr_content[3], attr_content[4], attr_content[5]);
  
 - if (copy_to_user(wrqu-data.pointer, go_devadd_str, 10 + 17))
 + if (copy_to_user(wrqu-data.pointer, go_devadd_str, 
 sizeof(go_devadd_str)))
   return -EFAULT;
   return ret;
  }
 --
 To unsubscribe from this list: send the line unsubscribe kernel-janitors in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [patch] Staging: vt6655-6: potential NULL dereference in hostap_disable_hostapd()

[walter harms]

Am 07.11.2013 08:55, schrieb Dan Carpenter:
 We fixed this to use free_netdev() instead of kfree() but unfortunately
 free_netdev() doesn't accept NULL pointers.  Smatch complains about
 this, it's not something I discovered through testing.
 
 Fixes: 3030d40b5036 ('staging: vt6655: use free_netdev instead of kfree')
 Fixes: 0a438d5b381e ('staging: vt6656: use free_netdev instead of kfree')
 Signed-off-by: Dan Carpenter dan.carpen...@oracle.com
 
 diff --git a/drivers/staging/vt6655/hostap.c b/drivers/staging/vt6655/hostap.c
 index aab0012..ab8b2ba 100644
 --- a/drivers/staging/vt6655/hostap.c
 +++ b/drivers/staging/vt6655/hostap.c
 @@ -143,7 +143,8 @@ static int hostap_disable_hostapd(PSDevice pDevice, int 
 rtnl_locked)
   DBG_PRT(MSG_LEVEL_DEBUG, KERN_INFO %s: Netdevice %s 
 unregistered\n,
   pDevice-dev-name, pDevice-apdev-name);
   }
 - free_netdev(pDevice-apdev);
 + if (pDevice-apdev)
 + free_netdev(pDevice-apdev);


perhaps the better way is to fix free_netdev() to make it behave like kfree() 
and friends.

just my 2 cents,
 wh

   pDevice-apdev = NULL;
   pDevice-bEnable8021x = false;
   pDevice-bEnableHostWEP = false;
 diff --git a/drivers/staging/vt6656/hostap.c b/drivers/staging/vt6656/hostap.c
 index ae1676d..67ba48b 100644
 --- a/drivers/staging/vt6656/hostap.c
 +++ b/drivers/staging/vt6656/hostap.c
 @@ -133,7 +133,8 @@ static int hostap_disable_hostapd(struct vnt_private 
 *pDevice, int rtnl_locked)
  DBG_PRT(MSG_LEVEL_DEBUG, KERN_INFO %s: Netdevice %s 
 unregistered\n,
  pDevice-dev-name, pDevice-apdev-name);
   }
 - free_netdev(pDevice-apdev);
 + if (pDevice-apdev)
 + free_netdev(pDevice-apdev);
   pDevice-apdev = NULL;
  pDevice-bEnable8021x = false;
  pDevice-bEnableHostWEP = false;
 --
 To unsubscribe from this list: send the line unsubscribe kernel-janitors in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 1/2] staging: lustre: Fix non-ANSI sparse warnings

[walter harms]

Am 28.07.2013 00:38, schrieb Emil Goode:
 This patch fixes a lot of non-ANSI sparse warnings by adding void to
 parameterless functions.
 
 Signed-off-by: Emil Goode emilgo...@gmail.com
 ---
 v2: A bit more specific commit message.
 
  .../lustre/lnet/klnds/socklnd/socklnd_lib-linux.c|6 +++---
  drivers/staging/lustre/lnet/lnet/api-ni.c|2 +-
  drivers/staging/lustre/lnet/selftest/console.c   |2 +-
  .../staging/lustre/lustre/libcfs/linux/linux-tracefile.c |   14 
 +++---
  drivers/staging/lustre/lustre/ptlrpc/pack_generic.c  |2 +-
  drivers/staging/lustre/lustre/ptlrpc/pinger.c|4 ++--
  6 files changed, 15 insertions(+), 15 deletions(-)
 
 diff --git a/drivers/staging/lustre/lnet/klnds/socklnd/socklnd_lib-linux.c 
 b/drivers/staging/lustre/lnet/klnds/socklnd/socklnd_lib-linux.c
 index 3e08fe2..76e442b 100644
 --- a/drivers/staging/lustre/lnet/klnds/socklnd/socklnd_lib-linux.c
 +++ b/drivers/staging/lustre/lnet/klnds/socklnd/socklnd_lib-linux.c
 @@ -325,20 +325,20 @@ ksocknal_lib_tunables_init ()
  }
  
  void
 -ksocknal_lib_tunables_fini ()
 +ksocknal_lib_tunables_fini(void)
  {
   if (ksocknal_tunables.ksnd_sysctl != NULL)
   unregister_sysctl_table(ksocknal_tunables.ksnd_sysctl);
  }

this is not needed as unregister_sysctl_table() can handle NULL

  #else
  int
 -ksocknal_lib_tunables_init ()
 +ksocknal_lib_tunables_init(void)
  {
   return 0;
  }
  
  void
 -ksocknal_lib_tunables_fini ()
 +ksocknal_lib_tunables_fini(void)
  {
  }
  #endif /* # if CONFIG_SYSCTL  !CFS_SYSFS_MODULE_PARM */
 diff --git a/drivers/staging/lustre/lnet/lnet/api-ni.c 
 b/drivers/staging/lustre/lnet/lnet/api-ni.c
 index 250c618..160a429 100644
 --- a/drivers/staging/lustre/lnet/lnet/api-ni.c
 +++ b/drivers/staging/lustre/lnet/lnet/api-ni.c
 @@ -1371,7 +1371,7 @@ EXPORT_SYMBOL(LNetNIInit);
   * \return always 0 for current implementation.
   */
  int
 -LNetNIFini()
 +LNetNIFini(void)
  {
   LNET_MUTEX_LOCK(the_lnet.ln_api_mutex);
  
 diff --git a/drivers/staging/lustre/lnet/selftest/console.c 
 b/drivers/staging/lustre/lnet/selftest/console.c
 index 78e8d04..09e4700 100644
 --- a/drivers/staging/lustre/lnet/selftest/console.c
 +++ b/drivers/staging/lustre/lnet/selftest/console.c
 @@ -1773,7 +1773,7 @@ lstcon_session_info(lst_sid_t *sid_up, int *key_up, 
 unsigned *featp,
  }
  
  int
 -lstcon_session_end()
 +lstcon_session_end(void)
  {
   lstcon_rpc_trans_t *trans;
   lstcon_group_t *grp;
 diff --git a/drivers/staging/lustre/lustre/libcfs/linux/linux-tracefile.c 
 b/drivers/staging/lustre/lustre/libcfs/linux/linux-tracefile.c
 index a500a0b..162beee 100644
 --- a/drivers/staging/lustre/lustre/libcfs/linux/linux-tracefile.c
 +++ b/drivers/staging/lustre/lustre/libcfs/linux/linux-tracefile.c
 @@ -51,7 +51,7 @@ char *cfs_trace_console_buffers[NR_CPUS][CFS_TCD_TYPE_MAX];
  
  struct rw_semaphore cfs_tracefile_sem;
  
 -int cfs_tracefile_init_arch()
 +int cfs_tracefile_init_arch(void)
  {
   inti;
   intj;
 @@ -96,7 +96,7 @@ out:
   return -ENOMEM;
  }
  
 -void cfs_tracefile_fini_arch()
 +void cfs_tracefile_fini_arch(void)
  {
   inti;
   intj;
 @@ -116,27 +116,27 @@ void cfs_tracefile_fini_arch()
   fini_rwsem(cfs_tracefile_sem);
  }
  
 -void cfs_tracefile_read_lock()
 +void cfs_tracefile_read_lock(void)
  {
   down_read(cfs_tracefile_sem);
  }
  
 -void cfs_tracefile_read_unlock()
 +void cfs_tracefile_read_unlock(void)
  {
   up_read(cfs_tracefile_sem);
  }
  
 -void cfs_tracefile_write_lock()
 +void cfs_tracefile_write_lock(void)
  {
   down_write(cfs_tracefile_sem);
  }
  
 -void cfs_tracefile_write_unlock()
 +void cfs_tracefile_write_unlock(void)
  {
   up_write(cfs_tracefile_sem);
  }
  
 -cfs_trace_buf_type_t cfs_trace_buf_idx_get()
 +cfs_trace_buf_type_t cfs_trace_buf_idx_get(void)
  {
   if (in_irq())
   return CFS_TCD_TYPE_IRQ;
 diff --git a/drivers/staging/lustre/lustre/ptlrpc/pack_generic.c 
 b/drivers/staging/lustre/lustre/ptlrpc/pack_generic.c
 index 648..9c20d0c 100644
 --- a/drivers/staging/lustre/lustre/ptlrpc/pack_generic.c
 +++ b/drivers/staging/lustre/lustre/ptlrpc/pack_generic.c
 @@ -115,7 +115,7 @@ int lustre_msg_check_version(struct lustre_msg *msg, 
 __u32 version)
  EXPORT_SYMBOL(lustre_msg_check_version);
  
  /* early reply size */
 -int lustre_msg_early_size()
 +int lustre_msg_early_size(void)
  {
   static int size = 0;
   if (!size) {
 diff --git a/drivers/staging/lustre/lustre/ptlrpc/pinger.c 
 b/drivers/staging/lustre/lustre/ptlrpc/pinger.c
 index f521251..79e7d08 100644
 --- a/drivers/staging/lustre/lustre/ptlrpc/pinger.c
 +++ b/drivers/staging/lustre/lustre/ptlrpc/pinger.c
 @@ -51,7 +51,7 @@ struct mutex pinger_mutex;
  static LIST_HEAD(pinger_imports);
  static struct list_head timeout_list = LIST_HEAD_INIT(timeout_list);
  
 -int ptlrpc_pinger_suppress_pings()
 +int ptlrpc_pinger_suppress_pings(void)