linux-kernel - Re: [PATCH 1/5] mfd:rtsx: Read vendor setting from config space

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20130715142046.GD5100@mwanda>
Date:	Mon, 15 Jul 2013 17:20:46 +0300
From:	Dan Carpenter <dan.carpenter@...cle.com>
To:	wei_wang@...lsil.com.cn
Cc:	sameo@...ux.intel.com, gregkh@...uxfoundation.org,
	adam.lee@...onical.com, linux-kernel@...r.kernel.org,
	rogerable@...ltek.com, devel@...uxdriverproject.org
Subject: Re: [PATCH 1/5] mfd:rtsx: Read vendor setting from config space

This code has a lot of undocumented magic flags and terrible naming
in it.

> +static void rtl8411_init_settings(struct rtsx_pcr *pcr)
> +{
> +	u32 val1;
> +	u8 val2;

These names are 100% useless.  "val" means nothing.  "1" means
nothing.  "2" means nothing.  In fact, val1 is REG1 and val2 is
REG3 as opposed to REG2 as one would expect from the name so it is
misleading.

> +
> +	rtsx_pci_read_config_dword(pcr, PCR_SETTING_REG1, &val1);
> +	dev_dbg(&(pcr->pci->dev), "Cfg 0x%x: 0x%x\n", PCR_SETTING_REG1, val1);
> +
> +	if (!(val1 & 0x1000000)) {

What is 0x1000000?  We do this test over and over so it should be
an inline function.

> +		pcr->aspm_val = (u8)((val1 >> 28) & 0x03);

The "_val" in "apsm_val" is useless.
What does "aspm" stand for?
No need to cast.  0-3 is less than 255 so casting to u8 is just
noise.
These shift masks are used over and over so they should be defined
as macros or inline functions in a header file:

#define rtl8411_reg_to_XXX(reg)		((reg >> 25) & 0x01)
#define rtl8411_reg_to_sdXXX(reg)	((reg >> 26) & 0x03)
#define rtl8411_reg_to_aspm(reg)	((reg >> 28) & 0x03)

(Choose your own names which make sense).

> +		pcr->sd30_drive_sel_1v8 = map_sd_drive((val1 >> 26) & 0x03);
> +		pcr->card_drive_sel &= 0x3F;
> +		pcr->card_drive_sel |= (u8)((val1 >> 25) & 0x01) << 6;

Again, no need to cast.  This should be a macro or an inline
function.

Pretty much the same comments apply throughout.  I'm not going to
comment on it line by line.

regards,
dan carpenter

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