Re: [PATCH 03/10] staging: wfx: format comments on 100 columns
On Fri, 2022-02-25 at 12:23 +0100, Jerome Pouiller wrote:
> From: Jérôme Pouiller
>
> A few comments were not yet formatted on 100 columns.
IMO, none of these changes are necessary or good changes.
80 columns is preferred.
Really comments should most always use 80 columns, and
only occasionally should code be more than 80 columns
and almost never should code be more than 100 columns.
> diff --git a/drivers/staging/wfx/data_tx.c b/drivers/staging/wfx/data_tx.c
[]
> @@ -117,9 +117,7 @@ static int wfx_tx_policy_get(struct wfx_vif *wvif, struct
> ieee80211_tx_rate *rat
> if (idx >= 0) {
> *renew = false;
> } else {
> - /* If policy is not found create a new one using the oldest
> - * entry in "free" list
> - */
> + /* If policy is not found create a new one using the oldest
> entry in "free" list */
> *renew = true;
> entry = list_entry(cache->free.prev, struct wfx_tx_policy,
> link);
> memcpy(entry->rates, wanted.rates, sizeof(entry->rates));
> @@ -494,9 +492,7 @@ void wfx_tx_confirm_cb(struct wfx_dev *wdev, const struct
> wfx_hif_cnf_tx *arg)
> wfx_tx_fill_rates(wdev, tx_info, arg);
> skb_trim(skb, skb->len - tx_priv->icv_size);
>
> - /* From now, you can touch to tx_info->status, but do not touch to
> - * tx_priv anymore
> - */
> + /* From now, you can touch to tx_info->status, but do not touch to
> tx_priv anymore */
> /* FIXME: use ieee80211_tx_info_clear_status() */
> memset(tx_info->rate_driver_data, 0, sizeof(tx_info->rate_driver_data));
> memset(tx_info->pad, 0, sizeof(tx_info->pad));
> diff --git a/drivers/staging/wfx/queue.c b/drivers/staging/wfx/queue.c
[]
> @@ -210,8 +210,8 @@ bool wfx_tx_queues_has_cab(struct wfx_vif *wvif)
> if (wvif->vif->type != NL80211_IFTYPE_AP)
> return false;
> for (i = 0; i < IEEE80211_NUM_ACS; ++i)
> - /* Note: since only AP can have mcast frames in queue and only
> - * one vif can be AP, all queued frames has same interface id
> + /* Note: since only AP can have mcast frames in queue and only
> one vif can be AP,
> + * all queued frames has same interface id
>*/
> if (!skb_queue_empty_lockless(>tx_queue[i].cab))
> return true;
> @@ -253,9 +253,8 @@ static struct sk_buff *wfx_tx_queues_get_skb(struct
> wfx_dev *wdev)
> skb = skb_dequeue([i]->cab);
> if (!skb)
> continue;
> - /* Note: since only AP can have mcast frames in queue
> - * and only one vif can be AP, all queued frames has
> - * same interface id
> + /* Note: since only AP can have mcast frames in queue
> and only one vif can
> + * be AP, all queued frames has same interface id
>*/
> hif = (struct wfx_hif_msg *)skb->data;
> WARN_ON(hif->interface != wvif->id);
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rtl8723bs/core: add spaces between operators
On Tue, 2021-03-16 at 20:05 +0800, Qiang Ma wrote:
> Add spaces between operators for a better readability
> in function 'rtw_seccalctkipmic'.
Perhaps better would be to refactor it a bit to
follow the comments. Something like:
---
drivers/staging/rtl8723bs/core/rtw_security.c | 24 ++--
1 file changed, 10 insertions(+), 14 deletions(-)
diff --git a/drivers/staging/rtl8723bs/core/rtw_security.c
b/drivers/staging/rtl8723bs/core/rtw_security.c
index a311595deafb..a30e1fa717af 100644
--- a/drivers/staging/rtl8723bs/core/rtw_security.c
+++ b/drivers/staging/rtl8723bs/core/rtw_security.c
@@ -405,30 +405,26 @@ void rtw_secgetmic(struct mic_data *pmicdata, u8 *dst)
void rtw_seccalctkipmic(u8 *key, u8 *header, u8 *data, u32 data_len, u8
*mic_code, u8 pri)
{
-
struct mic_data micdata;
u8 priority[4] = {0x0, 0x0, 0x0, 0x0};
+ int da_offset;
+ int sa_offset;
rtw_secmicsetkey(, key);
priority[0] = pri;
/* Michael MIC pseudo header: DA, SA, 3 x 0, Priority */
- if (header[1]&1) { /* ToDS == 1 */
- rtw_secmicappend(, [16], 6); /* DA */
- if (header[1]&2) /* From Ds == 1 */
- rtw_secmicappend(, [24], 6);
- else
- rtw_secmicappend(, [10], 6);
- } else {/* ToDS == 0 */
- rtw_secmicappend(, [4], 6); /* DA */
- if (header[1]&2) /* From Ds == 1 */
- rtw_secmicappend(, [16], 6);
- else
- rtw_secmicappend(, [10], 6);
+ if (header[1] & 1) { /* ToDS == 1 */
+ da_offset = 16;
+ sa_offset = (header[1] & 2) ? 24 : 10;
+ } else {/* ToDS == 0 */
+ da_offset = 4;
+ sa_offset = (header[1] & 2) ? 16 : 10;
}
+ rtw_secmicappend(, [da_offset], 6); /* DA */
+ rtw_secmicappend(, [sa_offset], 6); /* SA */
rtw_secmicappend(, [0], 4);
-
rtw_secmicappend(, data, data_len);
rtw_secgetmic(, mic_code);
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rtl8723bs: align comments
On Wed, 2021-03-10 at 20:48 +0300, Dan Carpenter wrote:
> You need to have a space character after the '*'.
Perhaps YA checkpatch test...
---
scripts/checkpatch.pl | 15 +++
1 file changed, 15 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index f42e5ba16d9b..0de553d52605 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3876,6 +3876,21 @@ sub process {
}
}
+# Independent comment lines should have a space after the comment initiator
+ if ($line =~ /^\+[ \t]*($;+)/) {#leading comment
+ my $comment = trim(substr($rawline, $-[1], $+[1] -
$-[1]));
+ if ($comment =~ m{^(/\*|\*/|\*|//)(.*)}) {
+ my $init = $1;
+ my $rest = $2;
+ if ($init =~ m{^(?:/\*|\*|//)$} &&
+ $rest ne '' &&
+ $rest !~ /^[\s\*=\-]/) {
+ WARN("COMMENT_STYLE",
+"Comments generally use whitespace
after the comment initiator\n" . $herecurr);
+ }
+ }
+ }
+
# check for missing blank lines after struct/union declarations
# with exceptions for various attributes and macros
if ($prevline =~ /^[\+ ]};?\s*$/ &&
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: gasket: fix indentation and lines ending with open parenthesis
On Sun, 2021-02-07 at 19:39 +0530, Mahak Gupta wrote:
> This patch fixes warnings of 'checkpatch.pl'. According to
> Linux coding guidelines, code should be aligned properly to
> match with open parenthesis and lines should not end with
> open parenthesis.
Perhaps try using temporaries to reduce line length when used multiple times...
Something like:
---
drivers/staging/gasket/gasket_ioctl.c | 31 +++
1 file changed, 19 insertions(+), 12 deletions(-)
diff --git a/drivers/staging/gasket/gasket_ioctl.c
b/drivers/staging/gasket/gasket_ioctl.c
index e3047d36d8db..3cb2227d5972 100644
--- a/drivers/staging/gasket/gasket_ioctl.c
+++ b/drivers/staging/gasket/gasket_ioctl.c
@@ -44,6 +44,7 @@ static int gasket_read_page_table_size(struct gasket_dev
*gasket_dev,
{
int ret = 0;
struct gasket_page_table_ioctl ibuf;
+ struct gasket_page_table *table;
if (copy_from_user(, argp, sizeof(struct gasket_page_table_ioctl)))
return -EFAULT;
@@ -51,8 +52,8 @@ static int gasket_read_page_table_size(struct gasket_dev
*gasket_dev,
if (ibuf.page_table_index >= gasket_dev->num_page_tables)
return -EFAULT;
- ibuf.size = gasket_page_table_num_entries(
- gasket_dev->page_table[ibuf.page_table_index]);
+ table = gasket_dev->page_table[ibuf.page_table_index];
+ ibuf.size = gasket_page_table_num_entries(table);
trace_gasket_ioctl_page_table_data(ibuf.page_table_index, ibuf.size,
ibuf.host_address,
@@ -70,6 +71,7 @@ static int gasket_read_simple_page_table_size(struct
gasket_dev *gasket_dev,
{
int ret = 0;
struct gasket_page_table_ioctl ibuf;
+ struct gasket_page_table *table;
if (copy_from_user(, argp, sizeof(struct gasket_page_table_ioctl)))
return -EFAULT;
@@ -77,8 +79,8 @@ static int gasket_read_simple_page_table_size(struct
gasket_dev *gasket_dev,
if (ibuf.page_table_index >= gasket_dev->num_page_tables)
return -EFAULT;
- ibuf.size =
-
gasket_page_table_num_simple_entries(gasket_dev->page_table[ibuf.page_table_index]);
+ table = gasket_dev->page_table[ibuf.page_table_index];
+ ibuf.size = gasket_page_table_num_simple_entries(table);
trace_gasket_ioctl_page_table_data(ibuf.page_table_index, ibuf.size,
ibuf.host_address,
@@ -97,6 +99,7 @@ static int gasket_partition_page_table(struct gasket_dev
*gasket_dev,
int ret;
struct gasket_page_table_ioctl ibuf;
uint max_page_table_size;
+ struct gasket_page_table *table;
if (copy_from_user(, argp, sizeof(struct gasket_page_table_ioctl)))
return -EFAULT;
@@ -107,8 +110,9 @@ static int gasket_partition_page_table(struct gasket_dev
*gasket_dev,
if (ibuf.page_table_index >= gasket_dev->num_page_tables)
return -EFAULT;
- max_page_table_size = gasket_page_table_max_size(
- gasket_dev->page_table[ibuf.page_table_index]);
+
+ table = gasket_dev->page_table[ibuf.page_table_index];
+ max_page_table_size = gasket_page_table_max_size(table);
if (ibuf.size > max_page_table_size) {
dev_dbg(gasket_dev->dev,
@@ -119,8 +123,7 @@ static int gasket_partition_page_table(struct gasket_dev
*gasket_dev,
mutex_lock(_dev->mutex);
- ret = gasket_page_table_partition(
- gasket_dev->page_table[ibuf.page_table_index], ibuf.size);
+ ret = gasket_page_table_partition(table, ibuf.size);
mutex_unlock(_dev->mutex);
return ret;
@@ -131,6 +134,7 @@ static int gasket_map_buffers(struct gasket_dev *gasket_dev,
struct gasket_page_table_ioctl __user *argp)
{
struct gasket_page_table_ioctl ibuf;
+ struct gasket_page_table *table;
if (copy_from_user(, argp, sizeof(struct gasket_page_table_ioctl)))
return -EFAULT;
@@ -142,12 +146,13 @@ static int gasket_map_buffers(struct gasket_dev
*gasket_dev,
if (ibuf.page_table_index >= gasket_dev->num_page_tables)
return -EFAULT;
- if
(gasket_page_table_are_addrs_bad(gasket_dev->page_table[ibuf.page_table_index],
+ table = gasket_dev->page_table[ibuf.page_table_index];
+ if (gasket_page_table_are_addrs_bad(table,
ibuf.host_address,
ibuf.device_address, ibuf.size))
return -EINVAL;
- return
gasket_page_table_map(gasket_dev->page_table[ibuf.page_table_index],
+ return gasket_page_table_map(table,
ibuf.host_address, ibuf.device_address,
ibuf.size / PAGE_SIZE);
}
@@ -157,6 +162,7 @@ static int gasket_unmap_buffers(struct gasket_dev
*gasket_dev,
Re: [PATCH] staging: gasket: fix indentation and lines ending with open parenthesis
On Sun, 2021-02-07 at 19:39 +0530, Mahak Gupta wrote:
> This patch fixes warnings of 'checkpatch.pl'. According to
> Linux coding guidelines, code should be aligned properly to
> match with open parenthesis and lines should not end with
> open parenthesis.
>
> Signed-off-by: Mahak Gupta
> ---
> drivers/staging/gasket/gasket_ioctl.c | 18 --
> 1 file changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/staging/gasket/gasket_ioctl.c
> b/drivers/staging/gasket/gasket_ioctl.c
> index e3047d36d8db..a966231bad42 100644
> --- a/drivers/staging/gasket/gasket_ioctl.c
> +++ b/drivers/staging/gasket/gasket_ioctl.c
> @@ -40,7 +40,7 @@ static int gasket_set_event_fd(struct gasket_dev
> *gasket_dev,
>
>
> /* Read the size of the page table. */
> static int gasket_read_page_table_size(struct gasket_dev *gasket_dev,
> - struct gasket_page_table_ioctl __user *argp)
> +struct gasket_page_table_ioctl __user
> *argp)
> {
> int ret = 0;
> struct gasket_page_table_ioctl ibuf;
> @@ -51,8 +51,7 @@ static int gasket_read_page_table_size(struct gasket_dev
> *gasket_dev,
> if (ibuf.page_table_index >= gasket_dev->num_page_tables)
> return -EFAULT;
>
>
> - ibuf.size = gasket_page_table_num_entries(
> - gasket_dev->page_table[ibuf.page_table_index]);
> + ibuf.size =
> gasket_page_table_num_entries(gasket_dev->page_table[ibuf.page_table_index]);
>
>
> trace_gasket_ioctl_page_table_data(ibuf.page_table_index, ibuf.size,
> ibuf.host_address,
> @@ -66,7 +65,7 @@ static int gasket_read_page_table_size(struct gasket_dev
> *gasket_dev,
>
>
> /* Read the size of the simple page table. */
> static int gasket_read_simple_page_table_size(struct gasket_dev *gasket_dev,
> - struct gasket_page_table_ioctl __user *argp)
> + struct gasket_page_table_ioctl
> __user *argp)
> {
> int ret = 0;
> struct gasket_page_table_ioctl ibuf;
> @@ -92,7 +91,7 @@ static int gasket_read_simple_page_table_size(struct
> gasket_dev *gasket_dev,
>
>
> /* Set the boundary between the simple and extended page tables. */
> static int gasket_partition_page_table(struct gasket_dev *gasket_dev,
> - struct gasket_page_table_ioctl __user *argp)
> +struct gasket_page_table_ioctl __user
> *argp)
> {
> int ret;
> struct gasket_page_table_ioctl ibuf;
> @@ -107,8 +106,8 @@ static int gasket_partition_page_table(struct gasket_dev
> *gasket_dev,
>
>
> if (ibuf.page_table_index >= gasket_dev->num_page_tables)
> return -EFAULT;
> - max_page_table_size = gasket_page_table_max_size(
> - gasket_dev->page_table[ibuf.page_table_index]);
> + max_page_table_size = gasket_page_table_max_size
> + (gasket_dev->page_table[ibuf.page_table_index]);
>
>
> if (ibuf.size > max_page_table_size) {
> dev_dbg(gasket_dev->dev,
> @@ -119,8 +118,7 @@ static int gasket_partition_page_table(struct gasket_dev
> *gasket_dev,
>
>
> mutex_lock(_dev->mutex);
>
>
> - ret = gasket_page_table_partition(
> - gasket_dev->page_table[ibuf.page_table_index], ibuf.size);
> + ret =
> gasket_page_table_partition(gasket_dev->page_table[ibuf.page_table_index],
> ibuf.size);
> mutex_unlock(_dev->mutex);
>
>
> return ret;
> @@ -183,7 +181,7 @@ static int gasket_unmap_buffers(struct gasket_dev
> *gasket_dev,
> * corresponding memory.
> */
> static int gasket_config_coherent_allocator(struct gasket_dev *gasket_dev,
> - struct gasket_coherent_alloc_config_ioctl __user *argp)
> + struct
> gasket_coherent_alloc_config_ioctl __user *argp)
> {
> int ret;
> struct gasket_coherent_alloc_config_ioctl ibuf;
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: octeon: convert all uses of strlcpy to strscpy in ethernet-mdio.c
On Sun, 2021-02-07 at 15:55 +0100, Greg KH wrote:
> On Sun, Feb 07, 2021 at 02:48:04PM +, Phillip Potter wrote:
> > Convert three calls to strlcpy inside the cvm_oct_get_drvinfo function
> > to strscpy calls. Fixes a style warning.
>
> Is it really safe to do this type of conversion here?
Yes. No locks are taken by either strlcpy or strscpy, and the conversion
is only done where the return value is unused.
strscpy is:
lib/string.c: * Preferred to strlcpy() since the API doesn't require reading
memory
lib/string.c- * from the src string beyond the specified "count" bytes, and
since
lib/string.c: * the return value is easier to error-check than strlcpy()'s.
lib/string.c- * In addition, the implementation is robust to the string
changing out
lib/string.c: * from underneath it, unlike the current strlcpy() implementation.
> If so, you need
> to provide evidence of it in the changelog, otherwise we could just do a
> search/replace across the whole kernel and be done with it :)
Yes please.
There's a cocci script for that in commit 75b1a8f9d62e
("ALSA: Convert strlcpy to strscpy when return value is unused")
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging: wimax: i2400m: fixing several coding style issues.
On Sun, 2021-01-31 at 23:42 +0300, dev.dra...@bk.ru wrote:
> diff --git a/drivers/staging/wimax/i2400m/rx.c
> b/drivers/staging/wimax/i2400m/rx.c
[]
> @@ -764,9 +763,9 @@ unsigned __i2400m_roq_update_ws(struct i2400m *i2400m,
> struct i2400m_roq *roq,
> new_nws);
> __skb_unlink(skb_itr, >queue);
> i2400m_net_erx(i2400m, skb_itr, roq_data_itr->cs);
> - }
> - else
> + } else {
> break; /* rest of packets all nsn_itr > nws */
> + }
> }
Rather than merely fixing what checkpatch complains about, it'd be
better to reverse the test above and break directly then unindent
the expected block.
---
drivers/staging/wimax/i2400m/rx.c | 16 ++--
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/drivers/staging/wimax/i2400m/rx.c
b/drivers/staging/wimax/i2400m/rx.c
index c9fb619a9e01..caeba9694b4e 100644
--- a/drivers/staging/wimax/i2400m/rx.c
+++ b/drivers/staging/wimax/i2400m/rx.c
@@ -757,16 +757,12 @@ unsigned __i2400m_roq_update_ws(struct i2400m *i2400m,
struct i2400m_roq *roq,
roq_data_itr = (struct i2400m_roq_data *) _itr->cb;
nsn_itr = __i2400m_roq_nsn(roq, roq_data_itr->sn);
/* NSN bounds assumed correct (checked when it was queued) */
- if (nsn_itr < new_nws) {
- d_printf(2, dev, "ERX: roq %p - release skb %p "
-"(nsn %u/%u new nws %u)\n",
-roq, skb_itr, nsn_itr, roq_data_itr->sn,
-new_nws);
- __skb_unlink(skb_itr, >queue);
- i2400m_net_erx(i2400m, skb_itr, roq_data_itr->cs);
- }
- else
- break; /* rest of packets all nsn_itr > nws */
+ if (nsn_itr >= new_nws)
+ break;
+ d_printf(2, dev, "ERX: roq %p - release skb %p (nsn %u/%u new
nws %u)\n",
+roq, skb_itr, nsn_itr, roq_data_itr->sn, new_nws);
+ __skb_unlink(skb_itr, >queue);
+ i2400m_net_erx(i2400m, skb_itr, roq_data_itr->cs);
}
roq->ws = sn;
return new_nws;
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: net: wimax: i2400m: fw: remove redundant initialization of variable result
On Thu, 2021-01-28 at 17:37 +, Colin King wrote: > From: Colin Ian King > > The variable result is being initialized with a value that is never > read and it is being updated later with a new value. The initialization > is redundant and can be removed. Isn't WIMAX dead? Shouldn't it be marked ORPHAN in MAINTAINERS? --- diff --git a/MAINTAINERS b/MAINTAINERS index caac09a3c5c9..922afd393cb6 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -19201,6 +19201,10 @@ S: Supported W: https://wireless.wiki.kernel.org/en/users/Drivers/wil6210 F: drivers/net/wireless/ath/wil6210/ +WIMAX +S: Orphan +F: drivers/staging/wimax/ + WINBOND CIR DRIVER M: David Härdeman S: Maintained ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v10] staging: fbtft: add tearing signal detect
> Comments are the exception to the "no spaces at the start of a line"
> rule. I was expecting that the kbuild-bot would send a Smatch warning
> for inconsistent indenting, but comments are not counted there either.
>
> I'm sort of surprised that we don't have checkpatch rule about the
> missing space characters. It should be: "/* Tearing Effect Line On */".
Maybe this but the "preceded by a tab" test is pretty noisy.
---
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 4f8494527139..72347e82d384 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3720,6 +3720,22 @@ sub process {
s/(\(\s*$Type\s*\))[ \t]+/$1/;
}
}
+
+# Comment styles
+# Initial comment only lines that have a leading space
+ if ($rawline =~ m{^\+([ \t]+)(?:/\*|//)} && $1 =~ / /) {
+ WARN("COMMENT_STYLE",
+"Initial comment lines should be indented only
with tabs\n" . $herecurr);
+# comments not aligned on tabs
+ } elsif ($rawline !~ m{^\+(?:/\*|//)} &&
+$rawline =~ m{^\+.*[^\t](?:/\*|//)}) {
+ CHK("COMMENT_STYLE",
+ "Comments should generally be preceded by a tab\n"
. $herecurr);
+ }
+
+# comment initiators should generally be followed by a space if using words
+ if ($rawline =~ m{^\+.*(?:/\*|//)\w}) {
+ WARN("COMMENT_STYLE",
+"Comment text should use a space after the comment
initiator\n" . $herecurr);
+ }
# Block comment styles
# Networking with an initial /*
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v10] staging: fbtft: add tearing signal detect
On Wed, 2021-01-27 at 17:49 +0300, Dan Carpenter wrote: > On Wed, Jan 27, 2021 at 03:25:20PM +0100, Greg KH wrote: > > Andy and Joe, there's something wrong here that is missing the fact that > > a line is being indented with spaces and not tabs in the patch > > at > > https://lore.kernel.org/r/1611754972-151016-1-git-send-email-zhangxuez...@gmail.com > > > > Any ideas what broke? > > /*Tearing Effect Line On*/ > > Comments are the exception to the "no spaces at the start of a line" > rule. I was expecting that the kbuild-bot would send a Smatch warning > for inconsistent indenting, but comments are not counted there either. > > I'm sort of surprised that we don't have checkpatch rule about the > missing space characters. It should be: "/* Tearing Effect Line On */". You could always write your own rule... checkpatch doesn't care if a comment looks like // or /*foobarfoobarfoobar*/ ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 4/4] phy: phy-hi3670-usb3: move driver from staging into phy
On Fri, 2021-01-15 at 09:10 +0100, Mauro Carvalho Chehab wrote: > The phy USB3 driver for Hisilicon 970 (hi3670) is ready > for mainstream. Mode it from staging into the main driver's > phy/ directory. [] > diff --git a/Documentation/devicetree/bindings/phy/phy-hi3670-usb3.yaml > b/Documentation/devicetree/bindings/phy/phy-hi3670-usb3.yaml [] > +++ b/Documentation/devicetree/bindings/phy/phy-hi3670-usb3.yaml > @@ -0,0 +1,72 @@ > +# SPDX-License-Identifier: GPL-2.0 > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/phy/hisilicon,hi3670-usb3.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Hisilicon Kirin970 USB PHY > + > +maintainers: > + - Mauro Carvalho Chehab > +description: |+ > + Bindings for USB3 PHY on HiSilicon Kirin 970. The cover letter for v3 says there's a blank link after maintainers: here but not found... ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 3/4] staging: hikey9xx: phy-hi3670-usb3.c: hi3670_is_abbclk_seleted() returns bool
On Thu, 2021-01-14 at 18:35 +0100, Mauro Carvalho Chehab wrote:
> Instead of using 1/0 for true/false, change the type to boolean
> and change the returned value.
[]
> diff --git a/drivers/staging/hikey9xx/phy-hi3670-usb3.c
> b/drivers/staging/hikey9xx/phy-hi3670-usb3.c
[]
> @@ -326,24 +326,24 @@ static int hi3670_phy_set_params(struct hi3670_priv
> *priv)
> return ret;
> }
>
> -static int hi3670_is_abbclk_seleted(struct hi3670_priv *priv)
> +static bool hi3670_is_abbclk_seleted(struct hi3670_priv *priv)
Presumably this should be "selected" not "seleted"
> {
> u32 reg;
>
> if (!priv->sctrl) {
> dev_err(priv->dev, "priv->sctrl is null!\n");
> - return 1;
> + return true;
> }
>
> if (regmap_read(priv->sctrl, SCTRL_SCDEEPSLEEPED, )) {
> dev_err(priv->dev, "SCTRL_SCDEEPSLEEPED read failed!\n");
> - return 1;
> + return true;
> }
>
> if ((reg & USB_CLK_SELECTED) == 0)
> - return 1;
> + return true;
>
> - return 0;
> + return false;
> }
if (foo)
return true;
return false;
should generally be consolidated into a single test.
So this is perhaps better as:
return (!(reg & USB_CLK_SELECTED));
But the return value seems backwards.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] media: atomisp: ov2722: replace hardcoded function name
On Wed, 2021-01-06 at 22:36 +0300, Dan Carpenter wrote:
> On Wed, Jan 06, 2021 at 10:25:26AM -0800, Joe Perches wrote:
> > On Wed, 2021-01-06 at 18:52 +0100, Greg Kroah-Hartman wrote:
> > > On Wed, Jan 06, 2021 at 07:43:42PM +0200, Filip Kolev wrote:
> > > > On 06-Jan-21 09:51, Greg Kroah-Hartman wrote:
> > > > > On Tue, Jan 05, 2021 at 10:29:18PM +0200, Filip Kolev wrote:
> > > > > > There is a debug message using hardcoded function name instead of
> > > > > > the
> > > > > > __func__ macro. Replace it.
> > > > > >
> > > > > > Report from checkpatch.pl on the file:
> > > > > >
> > > > > > WARNING: Prefer using '"%s...", __func__' to using 'ov2722_remove',
> > > > > > this function's name, in a string
> > > > > > + dev_dbg(>dev, "ov2722_remove...\n");
[]
> > There are quite a lot of these relatively useless function tracing like
> > uses in the kernel:
> >
> > $ git grep -P '"%s[\.\!]*\\n"\s*,\s*__func__\s*\)' | wc -l
> > 1065
>
> These are printing other stuff besides just the function name.
No, these are printing _only_ the function name.
> Maybe grep for '", __func__\)'?
No, that wouldn't work as there are many many uses like:
printk('%s: some string\n", __func__);
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] media: atomisp: ov2722: replace hardcoded function name
On Wed, 2021-01-06 at 18:52 +0100, Greg Kroah-Hartman wrote:
> On Wed, Jan 06, 2021 at 07:43:42PM +0200, Filip Kolev wrote:
> > On 06-Jan-21 09:51, Greg Kroah-Hartman wrote:
> > > On Tue, Jan 05, 2021 at 10:29:18PM +0200, Filip Kolev wrote:
> > > > There is a debug message using hardcoded function name instead of the
> > > > __func__ macro. Replace it.
> > > >
> > > > Report from checkpatch.pl on the file:
> > > >
> > > > WARNING: Prefer using '"%s...", __func__' to using 'ov2722_remove',
> > > > this function's name, in a string
> > > > + dev_dbg(>dev, "ov2722_remove...\n");
[]
> > > > diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
> > > > b/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
[]
> > > > @@ -1175,7 +1175,7 @@ static int ov2722_remove(struct i2c_client
> > > > *client)
> > > > struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > > > struct ov2722_device *dev = to_ov2722_sensor(sd);
> > > > - dev_dbg(>dev, "ov2722_remove...\n");
> > > > + dev_dbg(>dev, "%s...\n", __func__);
> > >
> > > dev_dbg() provides the function name already, and this is just a "trace"
> > > call, and ftrace should be used instead, so the whole line should be
> > > removed entirely.
> >
> > Thank you for the review!
> >
> > How do I go about this? Do I amend the patch and re-send as v2 or create a
> > new patch entirely?
>
> New patch entirely please.
There are quite a lot of these relatively useless function tracing like
uses in the kernel:
$ git grep -P '"%s[\.\!]*\\n"\s*,\s*__func__\s*\)' | wc -l
1065
Perhaps yet another checkpatch warning would be useful:
---
scripts/checkpatch.pl | 8
1 file changed, 8 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index e6857bdfcb2d..46b8ec8fe9e1 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5981,6 +5981,14 @@ sub process {
"Prefer using '\"%s...\", __func__' to using
'$context_function', this function's name, in a string\n" . $herecurr);
}
+# check for unnecessary function tracing like uses
+ if ($rawline =~
/^\+\s*$logFunctions\s*\([^"]*"%s[\.\!]*\\n"\s*,\s*__func__\s*\)\s*;\s*$/) {
+ if (WARN("TRACING_LOGGING",
+"Unnecessary ftrace-like logging - prefer
using ftrace\n" . $herecurr) &&
+ $fix) {
+fix_delete_line($fixlinenr, $rawline);
+ }
+ }
# check for spaces before a quoted newline
if ($rawline =~ /^.*\".*\s\\n/) {
if (WARN("QUOTED_WHITESPACE_BEFORE_NEWLINE",
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 8455/8455] staging: rtl8188eu: core: fixed a comment format issue.
On Mon, 2020-12-28 at 15:09 +0100, Greg KH wrote: > On Sat, Dec 19, 2020 at 02:43:12PM -0800, Daniel West wrote: > > Fixed a checkpatch warning: > > > > WARNING: Block comments use * on subsequent lines > > #4595: FILE: drivers/staging/rtl8188eu/core/rtw_mlme_ext.c:4595: > > +/ > > + > > > > The code is full of comments like this. Should the coding style > > be inforced here, even when there is a logic to the way the code > > was broken up? [] > > diff --git a/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c > > b/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c [] > > @@ -4591,11 +4591,10 @@ void mlmeext_sta_del_event_callback(struct adapter > > *padapter) > > } > > } > > > > > > -/ > > - > > -Following are the functions for the timer handlers > > - > > -*/ > > +/* > > + * > > + *Following are the functions for the timer handlers > > + */ > > Does that look correct? Make it all one line please. Just: /* timer handler functions */ would be simpler. And the code is mostly a horror to read. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: ralink-gdma: Fixed blank line coding style issue
On Wed, 2020-12-23 at 21:22 +0100, Ayoub Soussi wrote:
> Fixed coding style issue.
[]
> diff --git a/drivers/staging/ralink-gdma/ralink-gdma.c
> b/drivers/staging/ralink-gdma/ralink-gdma.c
[]
> @@ -122,6 +122,7 @@ struct gdma_dma_dev {
> struct gdma_data *data;
> void __iomem *base;
> struct tasklet_struct task;
> +
> volatile unsigned long chan_issued;
> atomic_t cnt;
This is presumably a checkpatch false positive.
checkpatch is not now nor never will be a perfect tool.
Please consider what you are doing and what the desired coding style is
before submitting patches.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rtl8723bs: clean up brace coding style issues
On Tue, 2020-12-22 at 15:17 +0100, Michael Straube wrote:
> Add missing braces around else arm of if else statement to clear
> style issues reported by checkpatch.
>
> CHECK: braces {} should be used on all arms of this statement
> CHECK: Unbalanced braces around else statement
[]
> diff --git a/drivers/staging/rtl8723bs/core/rtw_efuse.c
> b/drivers/staging/rtl8723bs/core/rtw_efuse.c
[]
> @@ -245,8 +245,9 @@ u16 Address)
> break;
> }
> return rtw_read8(Adapter, EFUSE_CTRL);
> - } else
> + } else {
> return 0xFF;
> + }
Instead, when you see a pattern like this it's generally
better to reverse whatever test is above this, return early
and unindent the block above the else.
Here that would be:
---
diff --git a/drivers/staging/rtl8723bs/core/rtw_efuse.c
b/drivers/staging/rtl8723bs/core/rtw_efuse.c
index 32ca10f01413..e5c3dba5c8ae 100644
--- a/drivers/staging/rtl8723bs/core/rtw_efuse.c
+++ b/drivers/staging/rtl8723bs/core/rtw_efuse.c
@@ -222,31 +222,31 @@ u16 Address)
EFUSE_GetEfuseDefinition(Adapter, EFUSE_WIFI,
TYPE_EFUSE_REAL_CONTENT_LEN, (void *), false);
- if (Address < contentLen) {/* E-fuse 512Byte */
- /* Write E-fuse Register address bit0~7 */
- temp = Address & 0xFF;
- rtw_write8(Adapter, EFUSE_CTRL+1, temp);
- Bytetemp = rtw_read8(Adapter, EFUSE_CTRL+2);
- /* Write E-fuse Register address bit8~9 */
- temp = ((Address >> 8) & 0x03) | (Bytetemp & 0xFC);
- rtw_write8(Adapter, EFUSE_CTRL+2, temp);
-
- /* Write 0x30[31]= 0 */
- Bytetemp = rtw_read8(Adapter, EFUSE_CTRL+3);
- temp = Bytetemp & 0x7F;
- rtw_write8(Adapter, EFUSE_CTRL+3, temp);
+ if (Address >= contentLen) /* E-fuse 512Byte */
+ return 0xFF;
- /* Wait Write-ready (0x30[31]= 1) */
+ /* Write E-fuse Register address bit0~7 */
+ temp = Address & 0xFF;
+ rtw_write8(Adapter, EFUSE_CTRL+1, temp);
+ Bytetemp = rtw_read8(Adapter, EFUSE_CTRL+2);
+ /* Write E-fuse Register address bit8~9 */
+ temp = ((Address >> 8) & 0x03) | (Bytetemp & 0xFC);
+ rtw_write8(Adapter, EFUSE_CTRL+2, temp);
+
+ /* Write 0x30[31]= 0 */
+ Bytetemp = rtw_read8(Adapter, EFUSE_CTRL+3);
+ temp = Bytetemp & 0x7F;
+ rtw_write8(Adapter, EFUSE_CTRL+3, temp);
+
+ /* Wait Write-ready (0x30[31]= 1) */
+ Bytetemp = rtw_read8(Adapter, EFUSE_CTRL+3);
+ while (!(Bytetemp & 0x80)) {
Bytetemp = rtw_read8(Adapter, EFUSE_CTRL+3);
- while (!(Bytetemp & 0x80)) {
- Bytetemp = rtw_read8(Adapter, EFUSE_CTRL+3);
- k++;
- if (k == 1000)
- break;
- }
- return rtw_read8(Adapter, EFUSE_CTRL);
- } else
- return 0xFF;
+ k++;
+ if (k == 1000)
+ break;
+ }
+ return rtw_read8(Adapter, EFUSE_CTRL);
} /* EFUSE_Read1Byte */
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: most: video: fixed a parentheses coding style issue.
On Fri, 2020-12-18 at 09:49 +, David Laight wrote:
> From: Joe Perches
> > Sent: 17 December 2020 23:58
> >
> > On Thu, 2020-12-17 at 15:45 -0800, Daniel West wrote:
> > > Fixed a coding style issue.
> >
> > It may pass checkpatch without warning, but it's uncommon kernel coding
> > style.
>
> checkpatch probably shouldn't complain about lines that end in (
> if they are function definitions.
Opinons vary.
Very few function declaration/definitions in the linux kernel use the
one line per argument style (gnu indent -bfde)
type function(
type argument1,
type argument2,
...
)
{
...
}
It probably shouldn't be encouraged.
> > or (> 80 columns)
> > static struct most_video_dev *get_comp_dev(struct most_interface *iface,
> > int channel_idx)
> Or shorten the variable/type names a bit so it all fits.
Always a possibility but probably not a good one here as even
renaming channel_idx to idx doesn't make it < 80 columns
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: most: video: fixed a parentheses coding style issue.
On Thu, 2020-12-17 at 15:45 -0800, Daniel West wrote:
> Fixed a coding style issue.
It may pass checkpatch without warning, but it's uncommon kernel coding style.
> diff --git a/drivers/staging/most/video/video.c
> b/drivers/staging/most/video/video.c
[]
> @@ -365,8 +365,8 @@ static const struct video_device comp_videodev_template =
> {
>
>
> /**/
>
>
> -static struct most_video_dev *get_comp_dev(
> - struct most_interface *iface, int channel_idx)
> +static struct most_video_dev *get_comp_dev
> + (struct most_interface *iface, int channel_idx)
This would be better using any of:
(most common)
static struct most_video_dev *get_comp_dev(struct most_interface *iface,
int channel_idx)
or (less common)
static struct most_video_dev *
get_comp_dev(struct most_interface *iface, int channel_idx)
or (> 80 columns)
static struct most_video_dev *get_comp_dev(struct most_interface *iface, int
channel_idx)
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging:rkvdec: Fixed "replace comma with semicolon" Warning:
On Fri, 2020-12-04 at 17:37 -0600, Travis Carter wrote: > drivers/staging/media/rkvdec/rkvdec.c You might consider using Julia Lawall's cocci script for all of drivers/staging or subsets of staging like drivers/staging/media/ https://lore.kernel.org/lkml/1601233948-11629-1-git-send-email-julia.law...@inria.fr/ ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Mon, 2020-11-23 at 07:58 -0800, James Bottomley wrote: > We're also complaining about the inability to recruit maintainers: > > https://www.theregister.com/2020/06/30/hard_to_find_linux_maintainers_says_torvalds/ > > And burn out: > > http://antirez.com/news/129 https://www.wired.com/story/open-source-coders-few-tired/ > What I'm actually trying to articulate is a way of measuring value of > the patch vs cost ... it has nothing really to do with who foots the > actual bill. It's unclear how to measure value in consistency. But one way that costs can be reduced is by automation and _not_ involving maintainers when the patch itself is provably correct. > One thesis I'm actually starting to formulate is that this continual > devaluing of maintainers is why we have so much difficulty keeping and > recruiting them. The linux kernel has something like 1500 different maintainers listed in the MAINTAINERS file. That's not a trivial number. $ git grep '^M:' MAINTAINERS | sort | uniq -c | wc -l 1543 $ git grep '^M:' MAINTAINERS| cut -f1 -d'<' | sort | uniq -c | wc -l 1446 I think the question you are asking is about trust and how it effects development. And back to that wired story, the actual number of what you might be considering to be maintainers is likely less than 10% of the listed numbers above. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Sun, 2020-11-22 at 11:12 -0800, James Bottomley wrote: > On Sun, 2020-11-22 at 10:25 -0800, Joe Perches wrote: > > On Sun, 2020-11-22 at 10:21 -0800, James Bottomley wrote: > > > Please tell me our reward for all this effort isn't a single > > > missing error print. > > > > There were quite literally dozens of logical defects found > > by the fallthrough additions. Very few were logging only. > > So can you give us the best examples (or indeed all of them if someone > is keeping score)? hopefully this isn't a US election situation ... Gustavo? Are you running for congress now? https://lwn.net/Articles/794944/ ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Sun, 2020-11-22 at 10:21 -0800, James Bottomley wrote: > Please tell me > our reward for all this effort isn't a single missing error print. There were quite literally dozens of logical defects found by the fallthrough additions. Very few were logging only. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Fri, 2020-11-20 at 12:21 -0600, Gustavo A. R. Silva wrote: > Hi all, > > This series aims to fix almost all remaining fall-through warnings in > order to enable -Wimplicit-fallthrough for Clang. > > In preparation to enable -Wimplicit-fallthrough for Clang, explicitly > add multiple break/goto/return/fallthrough statements instead of just > letting the code fall through to the next case. > > Notice that in order to enable -Wimplicit-fallthrough for Clang, this > change[1] is meant to be reverted at some point. So, this patch helps > to move in that direction. This was a bit hard to parse for a second or three. Thanks Gustavo. How was this change done? ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC] treewide: cleanup unreachable breaks
On Mon, 2020-10-19 at 12:42 -0700, Nick Desaulniers wrote: > On Sat, Oct 17, 2020 at 10:43 PM Greg KH wrote: > > On Sat, Oct 17, 2020 at 09:09:28AM -0700, t...@redhat.com wrote: > > > From: Tom Rix > > > > > > This is a upcoming change to clean up a new warning treewide. > > > I am wondering if the change could be one mega patch (see below) or > > > normal patch per file about 100 patches or somewhere half way by > > > collecting > > > early acks. > > > > Please break it up into one-patch-per-subsystem, like normal, and get it > > merged that way. > > > > Sending us a patch, without even a diffstat to review, isn't going to > > get you very far... > > Tom, > If you're able to automate this cleanup, I suggest checking in a > script that can be run on a directory. Then for each subsystem you > can say in your commit "I ran scripts/fix_whatever.py on this subdir." > Then others can help you drive the tree wide cleanup. Then we can > enable -Wunreachable-code-break either by default, or W=2 right now > might be a good idea. > > Ah, George (gbiv@, cc'ed), did an analysis recently of > `-Wunreachable-code-loop-increment`, `-Wunreachable-code-break`, and > `-Wunreachable-code-return` for Android userspace. From the review: > ``` > Spoilers: of these, it seems useful to turn on > -Wunreachable-code-loop-increment and -Wunreachable-code-return by > default for Android > ... > While these conventions about always having break arguably became > obsolete when we enabled -Wfallthrough, my sample turned up zero > potential bugs caught by this warning, and we'd need to put a lot of > effort into getting a clean tree. So this warning doesn't seem to be > worth it. > ``` > Looks like there's an order of magnitude of `-Wunreachable-code-break` > than the other two. > > We probably should add all 3 to W=2 builds (wrapped in cc-option). > I've filed https://github.com/ClangBuiltLinux/linux/issues/1180 to > follow up on. I suggest using W=1 as people that are doing cleanups generally use that and not W=123 or any other style. Every other use of W= is still quite noisy and these code warnings are relatively trivially to fix up. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [Ocfs2-devel] [RFC] treewide: cleanup unreachable breaks
On Sun, 2020-10-18 at 19:59 +0100, Matthew Wilcox wrote: > On Sat, Oct 17, 2020 at 09:09:28AM -0700, t...@redhat.com wrote: > > clang has a number of useful, new warnings see > > https://urldefense.com/v3/__https://clang.llvm.org/docs/DiagnosticsReference.html__;!!GqivPVa7Brio!Krxz78O3RKcB9JBMVo_F98FupVhj_jxX60ddN6tKGEbv_cnooXc1nnBmchm-e_O9ieGnyQ$ > > > > Please get your IT department to remove that stupidity. If you can't, > please send email from a non-Red Hat email address. I didn't get it this way, neither did lore. It's on your end. https://lore.kernel.org/lkml/20201017160928.12698-1-t...@redhat.com/ > I don't understand why this is a useful warning to fix. Precision in coding style intent and code minimization would be the biggest factors IMO. > What actual problem is caused by the code below? Obviously none. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [Cocci] [RFC] treewide: cleanup unreachable breaks
On Sat, 2020-10-17 at 20:21 +0200, Julia Lawall wrote: > On Sat, 17 Oct 2020, Joe Perches wrote: > > On Sat, 2020-10-17 at 09:09 -0700, t...@redhat.com wrote: > > > From: Tom Rix > > > > > > This is a upcoming change to clean up a new warning treewide. > > > I am wondering if the change could be one mega patch (see below) or > > > normal patch per file about 100 patches or somewhere half way by > > > collecting > > > early acks. > > > > > > clang has a number of useful, new warnings see > > > https://clang.llvm.org/docs/DiagnosticsReference.html > > > > > > This change cleans up -Wunreachable-code-break > > > https://clang.llvm.org/docs/DiagnosticsReference.html#wunreachable-code-break > > > for 266 of 485 warnings in this week's linux-next, allyesconfig on x86_64. > > > > Early acks/individual patches by subsystem would be good. > > Better still would be an automated cocci script. > > Coccinelle is not especially good at this, because it is based on control > flow, and a return or goto diverts the control flow away from the break. > A hack to solve the problem is to put an if around the return or goto, but > that gives the break a meaningless file name and line number. I collected > the following list, but it only has 439 results, so fewer than clang. But > maybe there are some files that are not considered by clang in the x86 > allyesconfig configuration. > > Probably checkpatch is the best solution here, since it is not > configuration sensitive and doesn't care about control flow. Likely the clang compiler is the best option here. It might be useful to add -Wunreachable-code-break to W=1 or just always enable it if it isn't already enabled. diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn index 95e4cdb94fe9..3819787579d5 100644 --- a/scripts/Makefile.extrawarn +++ b/scripts/Makefile.extrawarn @@ -32,6 +32,7 @@ KBUILD_CFLAGS += $(call cc-option, -Wunused-but-set-variable) KBUILD_CFLAGS += $(call cc-option, -Wunused-const-variable) KBUILD_CFLAGS += $(call cc-option, -Wpacked-not-aligned) KBUILD_CFLAGS += $(call cc-option, -Wstringop-truncation) +KBUILD_CFLAGS += $(call cc-option, -Wunreachable-code-break) # The following turn off the warnings enabled by -Wextra KBUILD_CFLAGS += -Wno-missing-field-initializers KBUILD_CFLAGS += -Wno-sign-compare (and thank you Tom for pushing this forward) checkpatch can't find instances like: case FOO: if (foo) return 1; else return 2; break; As it doesn't track flow and relies on the number of tabs to be the same for any goto/return and break; checkpatch will warn on: case FOO: ... goto bar; break; ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC] treewide: cleanup unreachable breaks
On Sat, 2020-10-17 at 09:09 -0700, t...@redhat.com wrote: > From: Tom Rix > > This is a upcoming change to clean up a new warning treewide. > I am wondering if the change could be one mega patch (see below) or > normal patch per file about 100 patches or somewhere half way by collecting > early acks. > > clang has a number of useful, new warnings see > https://clang.llvm.org/docs/DiagnosticsReference.html > > This change cleans up -Wunreachable-code-break > https://clang.llvm.org/docs/DiagnosticsReference.html#wunreachable-code-break > for 266 of 485 warnings in this week's linux-next, allyesconfig on x86_64. Early acks/individual patches by subsystem would be good. Better still would be an automated cocci script. The existing checkpatch test for UNNECESSARY_BREAK has a few too many false positives. >From a script run on next on July 28th: arch/arm/mach-s3c24xx/mach-rx1950.c:266: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/arm/nwfpe/fpa11_cprt.c:38: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/arm/nwfpe/fpa11_cprt.c:41: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/include/asm/mach-au1x00/au1000.h:684: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/include/asm/mach-au1x00/au1000.h:687: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/include/asm/mach-au1x00/au1000.h:690: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/include/asm/mach-au1x00/au1000.h:693: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/include/asm/mach-au1x00/au1000.h:697: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/include/asm/mach-au1x00/au1000.h:700: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/loongson2ef/common/cs5536/cs5536_isa.c:276: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/loongson2ef/common/cs5536/cs5536_isa.c:279: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/loongson2ef/common/cs5536/cs5536_isa.c:282: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/loongson2ef/common/cs5536/cs5536_isa.c:287: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/loongson2ef/common/cs5536/cs5536_isa.c:290: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/rb532/setup.c:76: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/rb532/setup.c:79: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/powerpc/include/asm/kvm_book3s_64.h:231: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/powerpc/include/asm/kvm_book3s_64.h:234: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/powerpc/include/asm/kvm_book3s_64.h:237: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/powerpc/include/asm/kvm_book3s_64.h:240: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/powerpc/net/bpf_jit_comp.c:455: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/powerpc/platforms/cell/spufs/switch.c:2047: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/powerpc/platforms/cell/spufs/switch.c:2077: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/sh/boards/mach-landisk/gio.c:111: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/x86/kernel/cpu/mce/core.c:1734: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/x86/kernel/cpu/mce/core.c:1738: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/x86/kernel/cpu/microcode/amd.c:218: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return drivers/acpi/utils.c:107: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return drivers/acpi/utils.c:132: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return drivers/acpi/utils.c:147: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return drivers/acpi/utils.c:158: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return drivers/ata/libata-scsi.c:3973: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return drivers/base/power/main.c:366: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return drivers/block/xen-blkback/blkback.c:1272: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return drivers/char/ipmi/ipmi_devintf.c:493: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return drivers/char/lp.c:625: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return drivers/char/mwave/mwavedd.c:406: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return
Re: [PATCH] staging: wfx: make a const array static, makes object smaller
On Fri, 2020-10-16 at 23:33 +0100, Colin King wrote: > From: Colin Ian King > > Don't populate const array filter_ies on the stack but instead > make it static. Makes the object code smaller by 261 bytes. > > Before: >text data bss dec hex filename > 21674 3166 448 2528862c8 drivers/staging/wfx/sta.o > > After: >text data bss dec hex filename > 21349 3230 448 2502761c3 drivers/staging/wfx/sta.o Thanks. It's odd to me it's so large a change as it's only 24 bytes of initialization. (3 entries, each 8 bytes) This line in the same function: hif_set_beacon_filter_table(wvif, 3, filter_ies); might as well use ARRAY_SIZE(filter_ies) instead of 3 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/2] media: staging: atomisp: Removed else branch in function
On Tue, 2020-10-06 at 21:04 +0300, Dan Carpenter wrote: > Code should generally do "error handling" instead of "success handling". Maybe something to add to coding-style (in '6} Functions' maybe?)... > That way the success path is always indented one tab and the error path > is indented two tabs. I like to say that the call and the error handling > are part of the same thing, but with success handling, it's like > do the call, do more stuff, go back to the error handling from the > earlier call. [] > Anyway, TLDR, please write it like this: > > if (on == 0) > return power_down(sd); > > ret = power_up(sd); > if (ret) > return ret; > > return gc0310_init(sd); Much nicer, thanks for taking the time to write it. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] Staging: nvec: nvec: fix double next comment
On Sun, 2020-09-27 at 09:35 -0700, Ryan Kosta wrote: > Changes since v1: > * Made commit message more clear > * Added description > Note: previous patch named > "[PATCH] fix double next comment in drivers/staging/nvec/nvec.c" > > 8--8< > Fixes a comment typo. Hi Ryan. That comment should be _below_ the --- separator line There's no need to double the nvec: word in the subject either. And please use the imperative. So the email message should look something like: - From: Subject: [PATCH V2] staging: nvec: Remove repeated word typo in a comment Fix a comment typo. Signed-off-by: --- V2: Add a commit description ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH RFT/RFC v2 11/47] staging: media: zoran: zoran_device.c: convert pr_x to pci_x
On Fri, 2020-09-25 at 18:30 +, Corentin Labbe wrote:
> Signed-off-by: Corentin Labbe
[]
> diff --git a/drivers/staging/media/zoran/zoran_device.c
> b/drivers/staging/media/zoran/zoran_device.c
[]
> @@ -198,15 +198,14 @@ void detect_guest_activity(struct zoran *zr)
[]
> for (i = 0; i < j; i++)
> - pr_info("%s: %6d: %d => 0x%02x\n", ZR_DEVNAME(zr),
> - change[i][0], change[i][1], change[i][2]);
> + pci_info(zr->pci_dev, "%6d: %d => 0x%02x\n", change[i][0],
> change[i][1], change[i][2]);
IMO: this change does little for readability or makes it worse.
Can you please keep to 80 columns where it's already 80 columns?
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/3] staging: rtl8192u: clean up blank line style issues
On Sat, 2020-09-19 at 17:08 +0200, Michael Straube wrote:
> Add missing and remove unnecessary blank lines to clear checkpatch
> issues.
unrelated trivia:
> diff --git a/drivers/staging/rtl8192u/r8192U_dm.c
> b/drivers/staging/rtl8192u/r8192U_dm.c
[]
> @@ -26,6 +26,7 @@ Major Change History:
> static u32 edca_setting_DL[HT_IOT_PEER_MAX] = {
> 0x5e4322, 0x5e4322, 0x5e4322, 0x604322, 0x00a44f, 0x5ea44f
> };
> +
> static u32 edca_setting_UL[HT_IOT_PEER_MAX] = {
> 0x5e4322, 0x00a44f, 0x5e4322, 0x604322, 0x5ea44f, 0x5ea44f
> };
These could be static const
Here and in drivers/staging/rtl8192e/rtl8192e/rtl_dm.c
and drivers/staging/rtl8723bs/hal/odm_EdcaTurboCheck.c
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC PATCH] Add bridge driver to connect sensors to CIO2 device via software nodes on ACPI platforms
On Thu, 2020-09-17 at 12:34 +0300, Dan Carpenter wrote:
> On Wed, Sep 16, 2020 at 10:36:18PM +0100, Daniel Scally wrote:
> > diff --git a/drivers/staging/media/ipu3/cio2-bridge.c
> > b/drivers/staging/media/ipu3/cio2-bridge.c
[]
> > + if (!dev->driver_data) {
> > + pr_info("ACPI match for %s, but it has no driver\n",
> > + supported_devices[i]);
>
> put_device(dev);
>
> > + continue;
> > + } else {
No need for an else either.
> > + pr_info("Found supported device %s\n",
> > + supported_devices[i]);
so this can be unindented too.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/2] staging: rtl8188eu: use __func__ in hal directory
On Thu, 2020-09-17 at 09:13 +0200, Michael Straube wrote:
> Use __func__ instead of hardcoded function names to clear
> checkpatch warnings.
[]
> diff --git a/drivers/staging/rtl8188eu/hal/odm.c
> b/drivers/staging/rtl8188eu/hal/odm.c
[]
> @@ -249,7 +249,7 @@ void odm_CommonInfoSelfUpdate(struct odm_dm_struct
> *pDM_Odm)
>
> void odm_CmnInfoInit_Debug(struct odm_dm_struct *pDM_Odm)
> {
> - ODM_RT_TRACE(pDM_Odm, ODM_COMP_COMMON, ODM_DBG_LOUD,
> ("odm_CmnInfoInit_Debug==>\n"));
> + ODM_RT_TRACE(pDM_Odm, ODM_COMP_COMMON, ODM_DBG_LOUD, ("%s==>\n",
> __func__));
> ODM_RT_TRACE(pDM_Odm, ODM_COMP_COMMON, ODM_DBG_LOUD,
> ("SupportPlatform=%d\n", pDM_Odm->SupportPlatform));
> ODM_RT_TRACE(pDM_Odm, ODM_COMP_COMMON, ODM_DBG_LOUD,
> ("SupportAbility=0x%x\n", pDM_Odm->SupportAbility));
> ODM_RT_TRACE(pDM_Odm, ODM_COMP_COMMON, ODM_DBG_LOUD,
> ("SupportInterface=%d\n", pDM_Odm->SupportInterface));
These ODM_RT_TRACE macro uses are rather ugly.
Perhaps a rename to odm_dbg would be better.
So would removing the unnecessary parentheses from
the macro uses and fixing the macro definition
Maybe using a perl script something like the below:
our $balanced_parens = qr/(\((?:[^\(\)]++|(?-1))*\))/;
sub deparenthesize {
my ($string) = @_;
return "" if (!defined($string));
while ($string =~ /^\s*\(.*\)\s*$/s) {
$string =~ s@^\s*\(\s*@@s;
$string =~ s@\s*\)\s*$@@s;
}
return $string;
}
foreach my $file (@ARGV) {
my $FILE;
my $count;
open($FILE, '<', $file) or die("Can't read $file $!\n");
undef $/;
my $text = (<$FILE>);
close($FILE);
$count = $text =~
s/ODM_RT_TRACE\s*\(\s*([^,]*),\s*ODM_COMP_(\w+)\s*,\s*ODM_DBG_(\w+)\s*,\s*($balanced_parens)\s*\)\s*;/"odm_dbg($1,
$2, $3, " . deparenthesize($4) . ");"/ge;
if ($count > 0) {
open($FILE, '>', $file) or die("Can't write $file $!\n");
print $FILE $text;
close($FILE);
}
}
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RESEND PATCH] staging: rtl8188eu: Fix else after return WARNING (checkpatch)
On Tue, 2020-09-15 at 14:20 +0300, Dan Carpenter wrote:
> On Mon, Sep 14, 2020 at 09:42:49AM -0700, Joe Perches wrote:
> > On Mon, 2020-09-14 at 17:57 +0300, Dan Carpenter wrote:
> > > On Sun, Sep 13, 2020 at 12:19:50PM +0530, Sohom Datta wrote:
> > > > > From 4c8c8f3ff7f4d711daea4ac3bb987fcecc7ef1ed Mon Sep 17 00:00:00 2001
> > > > From: Sohom
> > > > Date: Sat, 12 Sep 2020 18:04:56 +0530
> > > > Subject: [RESEND PATCH] staging: rtl8188eu: Fix else after return
> > > > WARNING
> > > > (checkpatch)
> > > >
> > > > Fixed:
> > > > WARNING: else is not generally useful after a break or return
> > > > 1636: FILE: ./rtw_recv.c:1636:
> > > > + return false;
> > > > + else
> > > >
> > > > Separated the return statement into a separate block since
> > > > it doesn't seem to depend on the SN_LESS explicity being false.
> > > >
> > > > Signed-off-by: Sohom
> > > > ---
> > > > drivers/staging/rtl8188eu/core/rtw_recv.c | 5 +++--
> > > > 1 file changed, 3 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/staging/rtl8188eu/core/rtw_recv.c
> > > > b/drivers/staging/rtl8188eu/core/rtw_recv.c
> > > > index 5fe7a0458dd2..5e81134ffb6d 100644
> > > > --- a/drivers/staging/rtl8188eu/core/rtw_recv.c
> > > > +++ b/drivers/staging/rtl8188eu/core/rtw_recv.c
> > > > @@ -1629,10 +1629,11 @@ static int enqueue_reorder_recvframe(struct
> > > > recv_reorder_ctrl *preorder_ctrl,
> > > > hdr = list_entry(plist, struct recv_frame, list);
> > > > pnextattrib = >attrib;
> > > >
> > > > + if (SN_EQUAL(pnextattrib->seq_num, pattrib->seq_num))
> > > > + return false;
> > > > +
> > > > if (SN_LESS(pnextattrib->seq_num, pattrib->seq_num))
> > > > plist = plist->next;
> > > > - else if (SN_EQUAL(pnextattrib->seq_num,
> > > > pattrib->seq_num))
> > > > - return false;
> > > > else
> > > > break;
> > > > }
> > >
> > > Checkpatch is just wrong here. Ignore it when it's wrong.
> >
> > It's not "wrong" here. It's making a suggestion.
> >
> > Perhaps read the SN_EQUAL and SN_LESS macros.
> >
> > a and b are both u16's here.
> >
> > drivers/staging/rtl8188eu/include/rtw_recv.h:#define SN_LESS(a, b)
> > (((a - b) & 0x800) != 0)
> > drivers/staging/rtl8188eu/include/rtw_recv.h:#define SN_EQUAL(a, b) (a
> > == b)
> >
> > Reordering works, perhaps it's just a question of
> > whether it's the most likely result of the test.
> >
> > This is in a while loop.
> >
> > If the expected test is really the most likely that
> > SN_LESS is true, then perhaps this loop could be
> > something like:
> >
> > if (SN_LESS(pnextattrib->seq_num, pattrib->seq_num)) {
> > plist = plist->next;
> > continue;
> > }
> > if (SN_EQUAL(pnextattrib->seq_num, pattrib->seq_num))
> > return false;
> > break;
> > }
> >
> > The real question is whether or not that's more readable.
> >
>
> It's not clear to me that any of these are more readable than the other.
>
> I see that someone broke the staging/rtl8712 version of this driver in
> June. See commit 98fe05e21a6e ("staging: rtl8712: Remove unnecesary
> else after return statement."). That patch went through LKML instead of
> going through the driver-devel list... :/
That's sad.
Then another question is whether either is more prone
to unintentional breakage by novice code readers being
guided by brainless scripts...
A bit of a pity as the original intent of the checkpatch
test was somewhat useful. Maybe it's outlived its value
though when used with -f files.
There aren't many of these left in the kernel.
Maybe it should be changed to work only on patches.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/3] staging: rtl8723bs: os_dep: fixed spacing around operators issue
On Mon, 2020-09-14 at 19:17 -0500, Ross Schmidt wrote:
> Fixed a coding style issue by adding spaces around operators in
> sdio_ops_linux.c to fix checkpatch checks.
Hello Ross.
If you want to do more than fix whitespace,
please look at the #define for DBG_871X.
All the uses with a KERN_ are broken
and should be modified as _dbgdump is just
printk and DRIVER_PREFIX is "RTL8723BS: "
so the KERN_ after the DRIVER_PREFIX.
That's just broken.
#define DBG_871X(...) do {\
_dbgdump(DRIVER_PREFIX __VA_ARGS__);\
} while (0)
Realistically, the DBG_871X macro family should just
use pr_debug and all the KERN_ uses should be
removed.
The define and uses of RT_TRACE should also just be
converted to use pr_debug or some other function
to perhaps reduce overall object size
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RESEND PATCH] staging: rtl8188eu: Fix else after return WARNING (checkpatch)
On Mon, 2020-09-14 at 17:57 +0300, Dan Carpenter wrote:
> On Sun, Sep 13, 2020 at 12:19:50PM +0530, Sohom Datta wrote:
> > > From 4c8c8f3ff7f4d711daea4ac3bb987fcecc7ef1ed Mon Sep 17 00:00:00 2001
> > From: Sohom
> > Date: Sat, 12 Sep 2020 18:04:56 +0530
> > Subject: [RESEND PATCH] staging: rtl8188eu: Fix else after return WARNING
> > (checkpatch)
> >
> > Fixed:
> > WARNING: else is not generally useful after a break or return
> > 1636: FILE: ./rtw_recv.c:1636:
> > + return false;
> > + else
> >
> > Separated the return statement into a separate block since
> > it doesn't seem to depend on the SN_LESS explicity being false.
> >
> > Signed-off-by: Sohom
> > ---
> > drivers/staging/rtl8188eu/core/rtw_recv.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/staging/rtl8188eu/core/rtw_recv.c
> > b/drivers/staging/rtl8188eu/core/rtw_recv.c
> > index 5fe7a0458dd2..5e81134ffb6d 100644
> > --- a/drivers/staging/rtl8188eu/core/rtw_recv.c
> > +++ b/drivers/staging/rtl8188eu/core/rtw_recv.c
> > @@ -1629,10 +1629,11 @@ static int enqueue_reorder_recvframe(struct
> > recv_reorder_ctrl *preorder_ctrl,
> > hdr = list_entry(plist, struct recv_frame, list);
> > pnextattrib = >attrib;
> >
> > + if (SN_EQUAL(pnextattrib->seq_num, pattrib->seq_num))
> > + return false;
> > +
> > if (SN_LESS(pnextattrib->seq_num, pattrib->seq_num))
> > plist = plist->next;
> > - else if (SN_EQUAL(pnextattrib->seq_num, pattrib->seq_num))
> > - return false;
> > else
> > break;
> > }
>
> Checkpatch is just wrong here. Ignore it when it's wrong.
It's not "wrong" here. It's making a suggestion.
Perhaps read the SN_EQUAL and SN_LESS macros.
a and b are both u16's here.
drivers/staging/rtl8188eu/include/rtw_recv.h:#define SN_LESS(a, b)
(((a - b) & 0x800) != 0)
drivers/staging/rtl8188eu/include/rtw_recv.h:#define SN_EQUAL(a, b) (a == b)
Reordering works, perhaps it's just a question of
whether it's the most likely result of the test.
This is in a while loop.
If the expected test is really the most likely that
SN_LESS is true, then perhaps this loop could be
something like:
if (SN_LESS(pnextattrib->seq_num, pattrib->seq_num)) {
plist = plist->next;
continue;
}
if (SN_EQUAL(pnextattrib->seq_num, pattrib->seq_num))
return false;
break;
}
The real question is whether or not that's more readable.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/5] staging: rtl8723bs: refactor cckrates{only}_included
On Sat, 2020-09-12 at 11:22 -0700, Joe Perches wrote:
> On Sat, 2020-09-12 at 10:45 +0200, Michael Straube wrote:
> > Refactor cckrates_included() and cckratesonly_included() to simplify
> > the code and improve readability.
[]
> diff --git a/drivers/staging/rtl8723bs/core/rtw_wlan_util.c
> b/drivers/staging/rtl8723bs/core/rtw_wlan_util.c
[]
> bool cckratesonly_included(unsigned char *rate, int ratelen)
> {
[]
> if (i <= 0)
if (ratelen <= 0) of course...
> return false;
>
> for (i = 0; i < ratelen; i++) {
> if (!is_cckrate(rate[i])
> return false;
> }
>
> return true;
> }
>
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/5] staging: rtl8723bs: refactor cckrates{only}_included
On Sat, 2020-09-12 at 10:45 +0200, Michael Straube wrote:
> Refactor cckrates_included() and cckratesonly_included() to simplify
> the code and improve readability.
>
> Signed-off-by: Michael Straube
> ---
> drivers/staging/rtl8723bs/core/rtw_wlan_util.c | 14 --
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/rtl8723bs/core/rtw_wlan_util.c
> b/drivers/staging/rtl8723bs/core/rtw_wlan_util.c
> index a5790a648a5b..4e0d86b2e2e0 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_wlan_util.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_wlan_util.c
> @@ -56,11 +56,12 @@ static u8 rtw_basic_rate_ofdm[3] = {
>
> int cckrates_included(unsigned char *rate, int ratelen)
> {
> - int i;
> + int i;
>
> for (i = 0; i < ratelen; i++) {
> - if rate[i]) & 0x7f) == 2) || (((rate[i]) & 0x7f) == 4) ||
> - (((rate[i]) & 0x7f) == 11) || (((rate[i]) & 0x7f) == 22))
> + u8 r = rate[i] & 0x7f;
> +
> + if (r == 2 || r == 4 || r == 11 || r == 22)
> return true;
> }
>
> @@ -69,11 +70,12 @@ int cckrates_included(unsigned char *rate, int ratelen)
>
> int cckratesonly_included(unsigned char *rate, int ratelen)
> {
> - int i;
> + int i;
>
> for (i = 0; i < ratelen; i++) {
> - if rate[i]) & 0x7f) != 2) && (((rate[i]) & 0x7f) != 4) &&
> - (((rate[i]) & 0x7f) != 11) && (((rate[i]) & 0x7f) != 22))
> + u8 r = rate[i] & 0x7f;
> +
> + if (r != 2 && r != 4 && r != 11 && r != 22)
> return false;
It would be simpler to add and use an inline like:
static bool is_cckrate(unsigned char rate)
{
rate &= 0x7f;
return rate == 2 || rate == 4 || rate == 11 || rate == 22;
}
so these could be
bool cckrates_included(unsigned char *rate, int ratelen)
{
int i;
for (i = 0; i < ratelen; i++) {
if (is_cckrate(rate[i])
return true;
}
return false;
}
bool cckratesonly_included(unsigned char *rate, int ratelen)
{
int i;
if (i <= 0)
return false;
for (i = 0; i < ratelen; i++) {
if (!is_cckrate(rate[i])
return false;
}
return true;
}
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: gdm724x: gdm_tty: replaced macro with a function
On Tue, 2020-09-01 at 22:16 +0200, Antoni Przybylik wrote:
> This approach is more elegant and prevents some problems related to
> macros such as operator precedence in expanded expression.
[]
> diff --git a/drivers/staging/gdm724x/gdm_tty.c
> b/drivers/staging/gdm724x/gdm_tty.c
[]
> @@ -36,6 +34,11 @@ static DEFINE_MUTEX(gdm_table_lock);
> static const char *DRIVER_STRING[TTY_MAX_COUNT] = {"GCTATC", "GCTDM"};
> static char *DEVICE_STRING[TTY_MAX_COUNT] = {"GCT-ATC", "GCT-DM"};
>
> +static int gdm_tty_ready(struct gdm *gdm)
> +{
> + return (gdm && gdm->tty_dev && gdm->port.count);
> +}
static bool gdm_tty_ready might be better.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 00/29] treewide: Convert comma separated statements
There are many comma separated statements in the kernel. See:https://lore.kernel.org/lkml/alpine.DEB.2.22.394.2008201856110.2524@hadrien/ Convert the comma separated statements that are in if/do/while blocks to use braces and semicolons. Many comma separated statements still exist but those are changes for another day. Joe Perches (29): coding-style.rst: Avoid comma statements alpha: Avoid comma separated statements ia64: Avoid comma separated statements sparc: Avoid comma separated statements ata: Avoid comma separated statements drbd: Avoid comma separated statements lp: Avoid comma separated statements dma-buf: Avoid comma separated statements drm/gma500: Avoid comma separated statements drm/i915: Avoid comma separated statements hwmon: (scmi-hwmon): Avoid comma separated statements Input: MT - Avoid comma separated statements bcache: Avoid comma separated statements media: Avoid comma separated statements mtd: Avoid comma separated statements 8390: Avoid comma separated statements fs_enet: Avoid comma separated statements wan: sbni: Avoid comma separated statements s390/tty3270: Avoid comma separated statements scai/arm: Avoid comma separated statements media: atomisp: Avoid comma separated statements video: fbdev: Avoid comma separated statements fuse: Avoid comma separated statements reiserfs: Avoid comma separated statements lib/zlib: Avoid comma separated statements lib: zstd: Avoid comma separated statements ipv6: fib6: Avoid comma separated statements sunrpc: Avoid comma separated statements tools: Avoid comma separated statements Documentation/process/coding-style.rst| 17 + arch/alpha/kernel/pci_iommu.c | 8 +- arch/alpha/oprofile/op_model_ev4.c| 22 +- arch/alpha/oprofile/op_model_ev5.c| 8 +- arch/ia64/kernel/smpboot.c| 7 +- arch/sparc/kernel/smp_64.c| 7 +- drivers/ata/pata_icside.c | 21 +- drivers/block/drbd/drbd_receiver.c| 6 +- drivers/char/lp.c | 6 +- drivers/dma-buf/st-dma-fence.c| 7 +- drivers/gpu/drm/gma500/mdfld_intel_display.c | 44 ++- drivers/gpu/drm/i915/gt/gen8_ppgtt.c | 8 +- drivers/gpu/drm/i915/gt/intel_gt_requests.c | 6 +- .../gpu/drm/i915/gt/selftest_workarounds.c| 6 +- drivers/gpu/drm/i915/intel_runtime_pm.c | 6 +- drivers/hwmon/scmi-hwmon.c| 6 +- drivers/input/input-mt.c | 11 +- drivers/md/bcache/bset.c | 12 +- drivers/md/bcache/sysfs.c | 6 +- drivers/media/i2c/msp3400-kthreads.c | 12 +- drivers/media/pci/bt8xx/bttv-cards.c | 6 +- drivers/media/pci/saa7134/saa7134-video.c | 7 +- drivers/mtd/devices/lart.c| 10 +- drivers/net/ethernet/8390/axnet_cs.c | 19 +- drivers/net/ethernet/8390/lib8390.c | 14 +- drivers/net/ethernet/8390/pcnet_cs.c | 6 +- .../ethernet/freescale/fs_enet/fs_enet-main.c | 11 +- drivers/net/wan/sbni.c| 101 +++--- drivers/s390/char/tty3270.c | 6 +- drivers/scsi/arm/cumana_2.c | 19 +- drivers/scsi/arm/eesox.c | 9 +- drivers/scsi/arm/powertec.c | 9 +- .../media/atomisp/pci/atomisp_subdev.c| 6 +- drivers/video/fbdev/tgafb.c | 12 +- fs/fuse/dir.c | 24 +- fs/reiserfs/fix_node.c| 36 ++- lib/zlib_deflate/deftree.c| 49 ++- lib/zstd/compress.c | 120 --- lib/zstd/fse_compress.c | 24 +- lib/zstd/huf_compress.c | 6 +- net/ipv6/ip6_fib.c| 12 +- net/sunrpc/sysctl.c | 6 +- tools/lib/subcmd/help.c | 10 +- tools/power/cpupower/utils/cpufreq-set.c | 14 +- tools/testing/selftests/vm/gup_benchmark.c| 18 +- tools/testing/selftests/vm/userfaultfd.c | 296 +++--- 46 files changed, 694 insertions(+), 382 deletions(-) -- 2.26.0 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 21/29] media: atomisp: Avoid comma separated statements
Use semicolons and braces.
Signed-off-by: Joe Perches
---
drivers/staging/media/atomisp/pci/atomisp_subdev.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/media/atomisp/pci/atomisp_subdev.c
b/drivers/staging/media/atomisp/pci/atomisp_subdev.c
index 6ba817f15655..52b9fb18c87f 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_subdev.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_subdev.c
@@ -410,8 +410,10 @@ int atomisp_subdev_set_selection(struct v4l2_subdev *sd,
if (atomisp_subdev_format_conversion(isp_sd,
isp_sd->capture_pad)
- && crop[pad]->width && crop[pad]->height)
- crop[pad]->width -= padding_w, crop[pad]->height -=
padding_h;
+ && crop[pad]->width && crop[pad]->height) {
+ crop[pad]->width -= padding_w;
+ crop[pad]->height -= padding_h;
+ }
/* if subdev type is SOC camera,we do not need to set DVS */
if (isp->inputs[isp_sd->input_curr].type == SOC_CAMERA)
--
2.26.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 10/49] staging: hikey9xx/gpu: add debug prints for this driver
On Wed, 2020-08-19 at 13:45 +0200, Mauro Carvalho Chehab wrote:
> From: Xiubin Zhang
>
> Add some debug prints on adv7535 and kirin_drm_drv.
bikeshed:
> diff --git a/drivers/staging/hikey9xx/gpu/hdmi/adv7535.c
> b/drivers/staging/hikey9xx/gpu/hdmi/adv7535.c
[]
> @@ -785,19 +786,25 @@ adv7511_detect(struct adv7511 *adv7511,
> {
> enum drm_connector_status status;
> unsigned int val;
> + unsigned int time = 0;
time is not a good name. Maybe rename to loops
> @@ -820,7 +827,32 @@ adv7511_detect(struct adv7511 *adv7511,
> }
> #endif
>
> + if (status == connector_status_disconnected) {
> + do {
> + ret = regmap_read(adv7511->regmap, ADV7511_REG_STATUS,
> );
> + if (ret < 0) {
> + DRM_ERROR("regmap_read fail, ret = %d \n", ret);
> + return connector_status_disconnected;
> + }
> +
> + if (val & ADV7511_STATUS_HPD) {
> + DRM_INFO("connected : regmap_read val = 0x%x
> \n", val);
> + status = connector_status_connected;
> + } else {
> + DRM_INFO("disconnected : regmap_read val = 0x%x
> \n", val);
> + status = connector_status_disconnected;
> + }
> + time ++;
> + mdelay(20);
> + } while (status == connector_status_disconnected && time < 10);
> + }
> +
> + if (time >= 10)
> + DRM_ERROR("Read connector status timout, time = %d \n", time);
No space necessary before ++
s/timout/timeout/
No space before the newline please in any of the DRM_ERROR
output messages.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 00/49] DRM driver for Hikey 970
On Wed, 2020-08-19 at 22:48 +0200, Sam Ravnborg wrote: > And sometimes checkpatch is just wrong. I'm interested in examples for when checkpatch is "just wrong". ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 3/3] media: atomisp: Only use trace_printk if allowed
On Thu, 2020-08-20 at 21:57 -0400, Steven Rostedt wrote: > On Fri, 21 Aug 2020 09:39:19 +0800 > Nicolas Boichat wrote: [] > > Some other approaches/ideas: > > 1. Filter all lkml messages that contain trace_printk. Already found > > 1 instance, and I can easily reply to those with a semi-canned answer, > > if I remember to check that filter regularly (not sustainable in the > > long run...). > > Added Joe Perches to the thread. > > We can update checkpatch.pl to complain about a trace_printk() that it > finds in the added code. Why? I don't see much value in a trace_printk checkpatch warning. tracing is still dependent on CONFIG_TRACING otherwise trace_printk is an if (0) ELI5 please. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 3/3] media: atomisp: Only use trace_printk if allowed
On Fri, 2020-08-21 at 10:42 +0800, Nicolas Boichat wrote:
> On Fri, Aug 21, 2020 at 10:36 AM Joe Perches wrote:
> > On Thu, 2020-08-20 at 21:57 -0400, Steven Rostedt wrote:
> > > On Fri, 21 Aug 2020 09:39:19 +0800
> > > Nicolas Boichat wrote:
> > []
> > > > Some other approaches/ideas:
> > > > 1. Filter all lkml messages that contain trace_printk. Already found
> > > > 1 instance, and I can easily reply to those with a semi-canned answer,
> > > > if I remember to check that filter regularly (not sustainable in the
> > > > long run...).
> > >
> > > Added Joe Perches to the thread.
> > >
> > > We can update checkpatch.pl to complain about a trace_printk() that it
> > > finds in the added code.
> >
> > Why?
> >
> > I don't see much value in a trace_printk checkpatch warning.
> > tracing is still dependent on CONFIG_TRACING otherwise
> > trace_printk is an if (0)
> >
> > ELI5 please.
>
> This is my "new" canned answer to this:
>
> Please do not use trace_printk in production code [1,2], it is only
> meant for debug use. Consider using trace events, or dev_dbg.
> [1] https://elixir.bootlin.com/linux/v5.8/source/kernel/trace/trace.c#L3158
> [2] https://elixir.bootlin.com/linux/v5.8/source/include/linux/kernel.h#L766
>
> I also had arguments in patch 2/3 notes:
>
> There's at least 3 reasons that I can come up with:
> 1. trace_printk introduces some overhead. [some users, e.g.
> Android/Chrome OS, want CONFIG_TRACING but _not_ that extra overhead]
> 2. If the kernel keeps adding always-enabled trace_printk, it will be
> much harder for developers to make use of trace_printk for debugging.
> 3. People may assume that trace_printk is for debugging only, and may
> accidentally output sensitive data (theoretical at this stage).
Perhaps make trace_printk dependent on #define DEBUG?
Something like:
---
include/linux/kernel.h | 7 +++
1 file changed, 7 insertions(+)
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 500def620d8f..6ca8f958df73 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -717,6 +717,7 @@ do {
\
* let gcc optimize the rest.
*/
+#ifdef DEBUG
#define trace_printk(fmt, ...) \
do { \
char ___STR[] = __stringify((__VA_ARGS__)); \
@@ -725,6 +726,12 @@ do {
\
else\
trace_puts(fmt);\
} while (0)
+#else
+#define trace_printk(fmt, ...) \
+do { \
+ __trace_printk_check_format(fmt, ##args); \
+} while (0)
+#endif
#define do_trace_printk(fmt, args...) \
do { \
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: kpc2000: kpc_dma: fix spelling mistake "for for" -> "for"
On Tue, 2020-08-18 at 17:46 +0100, Colin King wrote:
> There are a couple of duplicated "for" spelling mistakes in dev_err
> error messages. Fix these.
Might as well remove the messages instead.
> diff --git a/drivers/staging/kpc2000/kpc_dma/fileops.c
> b/drivers/staging/kpc2000/kpc_dma/fileops.c
[]
> @@ -53,7 +53,7 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
>
> acd = kzalloc(sizeof(*acd), GFP_KERNEL);
> if (!acd) {
> - dev_err(>ldev->pldev->dev, "Couldn't kmalloc space for
> for the aio data\n");
> + dev_err(>ldev->pldev->dev, "Couldn't kmalloc space for
> the aio data\n");
> return -ENOMEM;
> }
> memset(acd, 0x66, sizeof(struct aio_cb_data));
Also there's no reason to use kzalloc then a memset over
the alloc'ed memory or a different sizeof style for the
alloc then memset.
Setting the struct content to 0x66 does seem odd.
> @@ -69,7 +69,7 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
> acd->user_pages = kcalloc(acd->page_count, sizeof(struct page *),
> GFP_KERNEL);
> if (!acd->user_pages) {
> - dev_err(>ldev->pldev->dev, "Couldn't kmalloc space for
> for the page pointers\n");
> + dev_err(>ldev->pldev->dev, "Couldn't kmalloc space for
> the page pointers\n");
> rv = -ENOMEM;
> goto err_alloc_userpages;
> }
Maybe:
---
drivers/staging/kpc2000/kpc_dma/fileops.c | 10 --
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/kpc2000/kpc_dma/fileops.c
b/drivers/staging/kpc2000/kpc_dma/fileops.c
index dd716edd9b1b..c5330ad6b175 100644
--- a/drivers/staging/kpc2000/kpc_dma/fileops.c
+++ b/drivers/staging/kpc2000/kpc_dma/fileops.c
@@ -51,12 +51,11 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
ldev = priv->ldev;
- acd = kzalloc(sizeof(*acd), GFP_KERNEL);
- if (!acd) {
- dev_err(>ldev->pldev->dev, "Couldn't kmalloc space for
for the aio data\n");
+ acd = kmalloc(sizeof(*acd), GFP_KERNEL);
+ if (!acd)
return -ENOMEM;
- }
- memset(acd, 0x66, sizeof(struct aio_cb_data));
+
+ memset(acd, 0x66, sizeof(*acd));
acd->priv = priv;
acd->ldev = priv->ldev;
@@ -69,7 +68,6 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
acd->user_pages = kcalloc(acd->page_count, sizeof(struct page *),
GFP_KERNEL);
if (!acd->user_pages) {
- dev_err(>ldev->pldev->dev, "Couldn't kmalloc space for
for the page pointers\n");
rv = -ENOMEM;
goto err_alloc_userpages;
}
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 35/44] staging: regulator: hi6421v600-regulator: add a driver-specific debug macro
On Thu, 2020-08-13 at 12:10 +0200, Mauro Carvalho Chehab wrote:
> Em Wed, 12 Aug 2020 09:10:29 -0700
> Joe Perches escreveu:
>
> > On Wed, 2020-08-12 at 17:56 +0200, Mauro Carvalho Chehab wrote:
> > > Using dev_dbg() is not too nice, as, instead of printing the
> > > name of the regulator, it prints "regulator.", making
> > > harder to associate what is happening with each ldo line.
> > >
> > > So, add a debug-specific macro, which will print the rdev's
> > > name, just like the regulator core.
> >
> > Seems sensible, but trivially:
> >
> > > diff --git a/drivers/staging/hikey9xx/hi6421v600-regulator.c
> > > b/drivers/staging/hikey9xx/hi6421v600-regulator.c
> > []
> > > @@ -209,10 +212,10 @@ static unsigned int
> > > hi6421_spmi_regulator_get_optimum_mode(struct regulator_dev
> > > struct hi6421v600_regulator *sreg = rdev_get_drvdata(rdev);
> > >
> > > if (load_uA || ((unsigned int)load_uA > sreg->eco_uA)) {
> > > - dev_dbg(>dev, "%s: normal mode", __func__);
> > > + rdev_dbg(rdev, "normal mode");
> > > return REGULATOR_MODE_NORMAL;
> > > } else {
> > > - dev_dbg(>dev, "%s: idle mode", __func__);
> > > + rdev_dbg(rdev, "idle mode");
> >
> > missing terminating newlines
>
> As per request from Jonathan, I ended dropping those rdev_dbg()
> on a followup patch.
>
> Btw, after this changeset:
>
> commit 563873318d328d9bbab4b00dfd835ac7c7e28697
> Merge: 24532f768121 bfd8d3f23b51
> Author: Linus Torvalds
> Date: Mon Oct 10 09:29:50 2016 -0700
>
> Merge branch 'printk-cleanups'
>
> Merge my system logging cleanups, triggered by the broken '\n'
> patches.
>
> the printk lib will add a line feed if a "\n" is missing. I had
> to get rid of pr_cont() & friends on that time on media, due to that.
I know.
Message formats should still end in a newline.
Any other subsystem could use a pr_cont and that
could be added to any line without a terminating
newline.
Also any line without a newline will not be emitted
until another message is emitted.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 00/44] SPMI patches needed by Hikey 970
On Wed, 2020-08-12 at 15:47 -0300, Mauro Carvalho Chehab wrote: > Em Wed, 12 Aug 2020 10:13:51 -0700 > Joe Perches escreveu: > > > Perhaps these trivial bits on top: > > Sounds fine for me. Feel free to send it with your SOB, adding my reviewed by: > > Reviewed-by: Mauro Carvalho Chehab I don't know that your original series is going to be applied as-is so I think you should carry it. cheers, Joe ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 00/44] SPMI patches needed by Hikey 970
Perhaps these trivial bits on top:
---
drivers/staging/hikey9xx/hi6421-spmi-pmic.c | 5 +++--
drivers/staging/hikey9xx/hi6421v600-regulator.c | 6 +++---
drivers/staging/hikey9xx/hisi-spmi-controller.c | 21 +
3 files changed, 19 insertions(+), 13 deletions(-)
diff --git a/drivers/staging/hikey9xx/hi6421-spmi-pmic.c
b/drivers/staging/hikey9xx/hi6421-spmi-pmic.c
index 76766e7b8bf9..9d73458ca65a 100644
--- a/drivers/staging/hikey9xx/hi6421-spmi-pmic.c
+++ b/drivers/staging/hikey9xx/hi6421-spmi-pmic.c
@@ -99,7 +99,7 @@ int hi6421_spmi_pmic_write(struct hi6421_spmi_pmic *pmic, int
reg, u32 val)
EXPORT_SYMBOL(hi6421_spmi_pmic_write);
int hi6421_spmi_pmic_rmw(struct hi6421_spmi_pmic *pmic, int reg,
- u32 mask, u32 bits)
+u32 mask, u32 bits)
{
unsigned long flags;
u32 data;
@@ -130,7 +130,8 @@ static irqreturn_t hi6421_spmi_irq_handler(int irq, void
*data)
hi6421_spmi_pmic_write(pmic, (i + pmic->irq_addr), pending);
/* solve powerkey order */
- if ((i == HISI_IRQ_KEY_NUM) && ((pending & HISI_IRQ_KEY_VALUE)
== HISI_IRQ_KEY_VALUE)) {
+ if ((i == HISI_IRQ_KEY_NUM) &&
+ ((pending & HISI_IRQ_KEY_VALUE) == HISI_IRQ_KEY_VALUE)) {
generic_handle_irq(pmic->irqs[HISI_IRQ_KEY_DOWN]);
generic_handle_irq(pmic->irqs[HISI_IRQ_KEY_UP]);
pending &= (~HISI_IRQ_KEY_VALUE);
diff --git a/drivers/staging/hikey9xx/hi6421v600-regulator.c
b/drivers/staging/hikey9xx/hi6421v600-regulator.c
index 29ef6bcadd84..82635ff54a74 100644
--- a/drivers/staging/hikey9xx/hi6421v600-regulator.c
+++ b/drivers/staging/hikey9xx/hi6421v600-regulator.c
@@ -227,7 +227,7 @@ static int hi6421_spmi_dt_parse(struct platform_device
*pdev,
ret = of_property_read_u32(np, "reg", >enable_reg);
if (ret) {
- dev_err(dev, "missing reg property\nn");
+ dev_err(dev, "missing reg property\n");
return ret;
}
@@ -303,13 +303,13 @@ static int hi6421_spmi_dt_parse(struct platform_device
*pdev,
*/
rdesc->vsel_mask = (1 << (fls(rdesc->n_voltages) - 1)) - 1;
- dev_dbg(dev, "voltage selector settings: reg: 0x%x, mask: 0x%x",
+ dev_dbg(dev, "voltage selector settings: reg: 0x%x, mask: 0x%x\n",
rdesc->vsel_reg, rdesc->vsel_mask);
return 0;
}
-static struct regulator_ops hi6421_spmi_ldo_rops = {
+static const struct regulator_ops hi6421_spmi_ldo_rops = {
.is_enabled = hi6421_spmi_regulator_is_enabled,
.enable = hi6421_spmi_regulator_enable,
.disable = hi6421_spmi_regulator_disable,
diff --git a/drivers/staging/hikey9xx/hisi-spmi-controller.c
b/drivers/staging/hikey9xx/hisi-spmi-controller.c
index 583df10cbf1a..513d962b8bce 100644
--- a/drivers/staging/hikey9xx/hisi-spmi-controller.c
+++ b/drivers/staging/hikey9xx/hisi-spmi-controller.c
@@ -102,7 +102,7 @@ static int spmi_controller_wait_for_done(struct device *dev,
return 0;
}
udelay(1);
- } while(timeout--);
+ } while (timeout--);
dev_err(dev, "%s: timeout, status 0x%x\n", __func__, status);
return -ETIMEDOUT;
@@ -121,7 +121,7 @@ static int spmi_read_cmd(struct spmi_controller *ctrl,
if (bc > SPMI_CONTROLLER_MAX_TRANS_BYTES) {
dev_err(>dev,
- "spmi_controller supports 1..%d bytes per trans,
but:%ld requested",
+ "spmi_controller supports 1..%d bytes per trans,
but:%ld requested\n",
SPMI_CONTROLLER_MAX_TRANS_BYTES, bc);
return -EINVAL;
}
@@ -137,7 +137,7 @@ static int spmi_read_cmd(struct spmi_controller *ctrl,
op_code = SPMI_CMD_EXT_REG_READ_L;
break;
default:
- dev_err(>dev, "invalid read cmd 0x%x", opc);
+ dev_err(>dev, "invalid read cmd 0x%x\n", opc);
return -EINVAL;
}
@@ -157,7 +157,10 @@ static int spmi_read_cmd(struct spmi_controller *ctrl,
goto done;
for (i = 0; bc > i * SPMI_PER_DATAREG_BYTE; i++) {
- data = readl(spmi_controller->base + chnl_ofst +
SPMI_SLAVE_OFFSET * slave_id + SPMI_APB_SPMI_RDATA0_BASE_ADDR + i *
SPMI_PER_DATAREG_BYTE);
+ data = readl(spmi_controller->base + chnl_ofst +
+SPMI_SLAVE_OFFSET * slave_id +
+SPMI_APB_SPMI_RDATA0_BASE_ADDR +
+i * SPMI_PER_DATAREG_BYTE);
data = be32_to_cpu((__be32)data);
if ((bc - i * SPMI_PER_DATAREG_BYTE) >> 2) {
memcpy(buf, , sizeof(data));
@@ -194,7 +197,7 @@ static int spmi_write_cmd(struct spmi_controller *ctrl,
if (bc > SPMI_CONTROLLER_MAX_TRANS_BYTES) {
Re: [PATCH 06/44] staging: spmi: hisi-spmi-controller: use le32 macros where needed
On Wed, 2020-08-12 at 17:56 +0200, Mauro Carvalho Chehab wrote:
> Instead of manually using bswap_32(), just use the
> le32 macros.
Are you certain this code will now work on any endian cpu?
Maybe just use __swab32 instead
> diff --git a/drivers/staging/hikey9xx/hisi-spmi-controller.c
> b/drivers/staging/hikey9xx/hisi-spmi-controller.c
[]
> @@ -43,11 +42,6 @@
> #define SPMI_APB_SPMI_CMD_TYPE_OFFSET24
> #define SPMI_APB_SPMI_CMD_LENGTH_OFFSET 20
>
> -#define bswap_32(X) \
> -u32)(X) & 0xff00) >> 24) | \
> - (((u32)(X) & 0x00ff) >> 8) | \
> - (((u32)(X) & 0xff00) << 8) | \
> - (((u32)(X) & 0x00ff) << 24))
> #define SPMI_APB_SPMI_CMD_SLAVEID_OFFSET 16
> #define SPMI_APB_SPMI_CMD_ADDR_OFFSET0
>
> @@ -179,14 +173,15 @@ static int spmi_read_cmd(struct spmi_controller *ctrl,
>
> writel(cmd, spmi_controller->base + chnl_ofst +
> SPMI_APB_SPMI_CMD_BASE_ADDR);
>
> - rc = spmi_controller_wait_for_done(spmi_controller,
> spmi_controller->base, sid, addr);
> + rc = spmi_controller_wait_for_done(spmi_controller,
> +spmi_controller->base, sid, addr);
> if (rc)
> goto done;
>
> i = 0;
> do {
> data = readl(spmi_controller->base + chnl_ofst +
> SPMI_SLAVE_OFFSET * sid + SPMI_APB_SPMI_RDATA0_BASE_ADDR + i *
> SPMI_PER_DATAREG_BYTE);
> - data = bswap_32(data);
> + data = be32_to_cpu((__be32)data);
> if ((bc - i * SPMI_PER_DATAREG_BYTE) >> 2) {
> memcpy(buf, , sizeof(data));
> buf += sizeof(data);
> @@ -210,8 +205,7 @@ static int spmi_write_cmd(struct spmi_controller *ctrl,
> {
> struct spmi_controller_dev *spmi_controller =
> dev_get_drvdata(>dev);
> unsigned long flags;
> - u32 cmd;
> - u32 data = 0;
> + u32 cmd, data;
> int rc;
> u32 chnl_ofst = SPMI_CHANNEL_OFFSET * spmi_controller->channel;
> u8 op_code, i;
> @@ -246,7 +240,7 @@ static int spmi_write_cmd(struct spmi_controller *ctrl,
>
> i = 0;
> do {
> - memset(, 0, sizeof(data));
> + data = 0;
> if ((bc - i * SPMI_PER_DATAREG_BYTE) >> 2) {
> memcpy(, buf, sizeof(data));
> buf += sizeof(data);
> @@ -255,8 +249,8 @@ static int spmi_write_cmd(struct spmi_controller *ctrl,
> buf += (bc % SPMI_PER_DATAREG_BYTE);
> }
>
> - data = bswap_32(data);
> - writel(data, spmi_controller->base + chnl_ofst +
> SPMI_APB_SPMI_WDATA0_BASE_ADDR + SPMI_PER_DATAREG_BYTE * i);
> + writel((u32)cpu_to_be32(data),
> +spmi_controller->base + chnl_ofst +
> SPMI_APB_SPMI_WDATA0_BASE_ADDR + SPMI_PER_DATAREG_BYTE * i);
> i++;
> } while (bc > i * SPMI_PER_DATAREG_BYTE);
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 10/44] staging: spmi: hisi-spmi-controller: do some code cleanups
On Wed, 2020-08-12 at 17:56 +0200, Mauro Carvalho Chehab wrote:
> There are several minor things that can be cleanup in
> order to make this driver more prepared for leaving staging.
trivial style notes:
> diff --git a/drivers/staging/hikey9xx/hisi-spmi-controller.c
> b/drivers/staging/hikey9xx/hisi-spmi-controller.c
[]
> @@ -106,14 +83,13 @@ static int spmi_controller_wait_for_done(struct device
> *dev,
>struct spmi_controller_dev *ctrl_dev,
>void __iomem *base, u8 sid, u16 addr)
> {
> - u32 status = 0;
> u32 timeout = SPMI_CONTROLLER_TIMEOUT_US;
> - u32 offset;
> + u32 status, offset;
>
> offset = SPMI_APB_SPMI_STATUS_BASE_ADDR;
> offset += SPMI_CHANNEL_OFFSET * ctrl_dev->channel + SPMI_SLAVE_OFFSET *
> sid;
>
> - while (timeout--) {
> + do {
> status = readl(base + offset);
>
> if (status & SPMI_APB_TRANS_DONE) {
> @@ -126,21 +102,21 @@ static int spmi_controller_wait_for_done(struct device
> *dev,
> return 0;
> }
> udelay(1);
> - }
> + } while(timeout--);
Odd spacing.
Two spaces after close brace, none before open parenthesis.
> @@ -237,14 +217,13 @@ static int spmi_write_cmd(struct spmi_controller *ctrl,
[]
> - i = 0;
> - do {
> + for (i = 0; bc > i * SPMI_PER_DATAREG_BYTE; i++) {
> data = 0;
> if ((bc - i * SPMI_PER_DATAREG_BYTE) >> 2) {
> memcpy(, buf, sizeof(data));
> @@ -256,22 +235,22 @@ static int spmi_write_cmd(struct spmi_controller *ctrl,
>
> writel((u32)cpu_to_be32(data),
> spmi_controller->base + chnl_ofst +
> SPMI_APB_SPMI_WDATA0_BASE_ADDR + SPMI_PER_DATAREG_BYTE * i);
> - i++;
> - } while (bc > i * SPMI_PER_DATAREG_BYTE);
> + };
Unnecessary semicolon after for loop close brace.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 20/44] staging: mfd: hi6421-spmi-pmic: fix some coding style issues
On Wed, 2020-08-12 at 17:56 +0200, Mauro Carvalho Chehab wrote:
> Checkpatch complains about some minor issues inside this
> driver that were not addressed by the previous patch.
>
> Address them.
[]
> diff --git a/include/linux/mfd/hi6421-spmi-pmic.h
> b/include/linux/mfd/hi6421-spmi-pmic.h
[]
> @@ -38,7 +38,8 @@ struct hi6421_spmi_pmic {
> unsigned int*irqs;
> int irqnum;
> int irqarray;
> - struct hi6421_spmi_irq_mask_infoirq_mask_addr;
> +
> + struct hi6421_spmi_irq_mask_infoirq_mask_addr;
> struct hi6421_spmi_irq_info irq_addr;
> };
Was adding a blank line one of checkpatch complaints?
I doubt it.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 35/44] staging: regulator: hi6421v600-regulator: add a driver-specific debug macro
On Wed, 2020-08-12 at 17:56 +0200, Mauro Carvalho Chehab wrote:
> Using dev_dbg() is not too nice, as, instead of printing the
> name of the regulator, it prints "regulator.", making
> harder to associate what is happening with each ldo line.
>
> So, add a debug-specific macro, which will print the rdev's
> name, just like the regulator core.
Seems sensible, but trivially:
> diff --git a/drivers/staging/hikey9xx/hi6421v600-regulator.c
> b/drivers/staging/hikey9xx/hi6421v600-regulator.c
[]
> @@ -209,10 +212,10 @@ static unsigned int
> hi6421_spmi_regulator_get_optimum_mode(struct regulator_dev
> struct hi6421v600_regulator *sreg = rdev_get_drvdata(rdev);
>
> if (load_uA || ((unsigned int)load_uA > sreg->eco_uA)) {
> - dev_dbg(>dev, "%s: normal mode", __func__);
> + rdev_dbg(rdev, "normal mode");
> return REGULATOR_MODE_NORMAL;
> } else {
> - dev_dbg(>dev, "%s: idle mode", __func__);
> + rdev_dbg(rdev, "idle mode");
missing terminating newlines
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: wfx: fixed misspelled word in comment
On Tue, 2020-08-04 at 16:07 +0530, Aditya Bansal wrote: > Should I correct all the instances of the "careful" and submit the > patch including both "carefull" and "function" word correct? or only > for the "carefull"? I have already submitted one for the "funcion" > word. I suggest submitting a V2 patch with both fixes. Generally it's better to submit all spelling fixes in a single file at once, especially when multiple spelling fixes are on the same line. Also, it's generally better to submit separate patches for individual files. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: wfx: fixed misspelled word in comment
On Tue, 2020-08-04 at 08:24 -0700, Randy Dunlap wrote: > On 8/4/20 7:58 AM, Aditya Bansal wrote: > > From: Aditya Bansal > > > > Subject: [PATCH] fixed typo in driver/staging/wfx/hif_tx.c file > > > > Correct the spelling of function > > > > Signed-off-by: Aditya Bansal > > --- > > > > diff --git a/drivers/staging/wfx/hif_tx.c b/drivers/staging/wfx/hif_tx.c > > index 5110f9b93762..6a485fa5b72b 100644 > > --- a/drivers/staging/wfx/hif_tx.c > > +++ b/drivers/staging/wfx/hif_tx.c > > @@ -125,7 +125,7 @@ int wfx_cmd_send(struct wfx_dev *wdev, struct hif_msg > > *request, > > > > // This function is special. After HIF_REQ_ID_SHUT_DOWN, chip won't reply > > to any > > // request anymore. We need to slightly hack struct wfx_hif_cmd for that > > job. Be > > -// carefull to only call this funcion during device unregister. > > +// carefull to only call this function during device unregister. > > careful And if you could do all of them: $ git grep -w -i -n carefull arch/m68k/coldfire/pci.c:34: * We need to be carefull probing on bus 0 (directly connected to host arch/openrisc/kernel/head.S:289: * a bit more carefull (if we have a PT_SP or current pointer drivers/staging/wfx/debug.c:302:// Be carefull, write() is waiting for a full message while read() drivers/staging/wfx/hif_tx.c:128:// carefull to only call this funcion during device unregister. fs/ceph/inode.c:1475: /* parent inode is not locked, be carefull */ ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rts5208: clear alignment style issues
On Sun, 2020-08-02 at 00:00 +0300, Tomer Samara wrote:
> Clear checkpatch alignment style issues in rtsx_transport.c.
> CHECK: Alignment should match open parenthesis
[]
> diff --git a/drivers/staging/rts5208/rtsx_transport.c
> b/drivers/staging/rts5208/rtsx_transport.c
[]
> @@ -678,7 +678,7 @@ static int rtsx_transfer_buf(struct rtsx_chip *chip, u8
> card, void *buf,
>
> /* Wait for TRANS_OK_INT */
> timeleft = wait_for_completion_interruptible_timeout(_done,
> - msecs_to_jiffies(timeout));
> +
> msecs_to_jiffies(timeout));
> if (timeleft <= 0) {
> dev_dbg(rtsx_dev(chip), "Timeout (%s %d)\n",
> __func__, __LINE__);
Always try to improve the code rather than just
shut up checkpatch warnings.
Perhaps it's more sensible to centralize the uses of
wait_for_completion_interruptible_timeout.
Something like:
---
drivers/staging/rts5208/rtsx_transport.c | 36 +---
1 file changed, 14 insertions(+), 22 deletions(-)
diff --git a/drivers/staging/rts5208/rtsx_transport.c
b/drivers/staging/rts5208/rtsx_transport.c
index 5f1eefe80f1e..269ff1be7cba 100644
--- a/drivers/staging/rts5208/rtsx_transport.c
+++ b/drivers/staging/rts5208/rtsx_transport.c
@@ -222,12 +222,18 @@ void rtsx_send_cmd_no_wait(struct rtsx_chip *chip)
rtsx_writel(chip, RTSX_HCBCTLR, val);
}
+static inline bool rtsx_wait(struct completion *comp, int timeout)
+{
+ unsigned long t = msecs_to_jiffies(timeout);
+
+ return wait_for_completion_interruptible_timeout(comp, t) > 0;
+}
+
int rtsx_send_cmd(struct rtsx_chip *chip, u8 card, int timeout)
{
struct rtsx_dev *rtsx = chip->rtsx;
struct completion trans_done;
u32 val = BIT(31);
- long timeleft;
int err = 0;
if (card == SD_CARD)
@@ -257,9 +263,8 @@ int rtsx_send_cmd(struct rtsx_chip *chip, u8 card, int
timeout)
spin_unlock_irq(>reg_lock);
/* Wait for TRANS_OK_INT */
- timeleft = wait_for_completion_interruptible_timeout(
- _done, msecs_to_jiffies(timeout));
- if (timeleft <= 0) {
+
+ if (!rtsx_wait(_done, timeout)) {
dev_dbg(rtsx_dev(chip), "chip->int_reg = 0x%x\n",
chip->int_reg);
err = -ETIMEDOUT;
@@ -322,7 +327,6 @@ static int rtsx_transfer_sglist_adma_partial(struct
rtsx_chip *chip, u8 card,
u8 dir;
int sg_cnt, i, resid;
int err = 0;
- long timeleft;
struct scatterlist *sg_ptr;
u32 val = TRIG_DMA;
@@ -419,9 +423,7 @@ static int rtsx_transfer_sglist_adma_partial(struct
rtsx_chip *chip, u8 card,
spin_unlock_irq(>reg_lock);
- timeleft = wait_for_completion_interruptible_timeout(
- _done, msecs_to_jiffies(timeout));
- if (timeleft <= 0) {
+ if (!rtsx_wait(_done, timeout)) {
dev_dbg(rtsx_dev(chip), "Timeout (%s %d)\n",
__func__, __LINE__);
dev_dbg(rtsx_dev(chip), "chip->int_reg = 0x%x\n",
@@ -443,9 +445,7 @@ static int rtsx_transfer_sglist_adma_partial(struct
rtsx_chip *chip, u8 card,
if (rtsx->trans_result == TRANS_NOT_READY) {
init_completion(_done);
spin_unlock_irq(>reg_lock);
- timeleft = wait_for_completion_interruptible_timeout(
- _done, msecs_to_jiffies(timeout));
- if (timeleft <= 0) {
+ if (!rtsx_wait(_done, timeout)) {
dev_dbg(rtsx_dev(chip), "Timeout (%s %d)\n",
__func__, __LINE__);
dev_dbg(rtsx_dev(chip), "chip->int_reg = 0x%x\n",
@@ -486,7 +486,6 @@ static int rtsx_transfer_sglist_adma(struct rtsx_chip
*chip, u8 card,
u8 dir;
int buf_cnt, i;
int err = 0;
- long timeleft;
struct scatterlist *sg_ptr;
if (!sg || (num_sg <= 0))
@@ -563,9 +562,7 @@ static int rtsx_transfer_sglist_adma(struct rtsx_chip
*chip, u8 card,
spin_unlock_irq(>reg_lock);
- timeleft = wait_for_completion_interruptible_timeout(
- _done, msecs_to_jiffies(timeout));
- if (timeleft <= 0) {
+ if (!rtsx_wait(_done, timeout)) {
dev_dbg(rtsx_dev(chip), "Timeout (%s %d)\n",
__func__, __LINE__);
dev_dbg(rtsx_dev(chip), "chip->int_reg = 0x%x\n",
@@ -590,9 +587,7 @@ static int rtsx_transfer_sglist_adma(struct rtsx_chip
*chip, u8 card,
if (rtsx->trans_result == TRANS_NOT_READY) {
init_completion(_done);
spin_unlock_irq(>reg_lock);
- timeleft = wait_for_completion_interruptible_timeout(
- _done, msecs_to_jiffies(timeout));
- if (timeleft <= 0) {
+ if
Re: [PATCH v2] staging: r8188eu: replace rtw_netdev_priv define with inline function
On Sat, 2020-08-01 at 19:52 +0300, Ivan Safonov wrote:
> The function guarantees type checking of arguments and return value.
>
> Result of rtw_netdev_priv macro can be assigned to pointer
> with incompatible type without warning. The function allow compiler
> to perform this check.
[]
> diff --git a/drivers/staging/rtl8188eu/include/osdep_service.h
> b/drivers/staging/rtl8188eu/include/osdep_service.h
[]
> @@ -71,8 +71,11 @@ struct rtw_netdev_priv_indicator {
> };
> struct net_device *rtw_alloc_etherdev_with_old_priv(void *old_priv);
>
> -#define rtw_netdev_priv(netdev) \
> - (((struct rtw_netdev_priv_indicator *)netdev_priv(netdev))->priv)
> +static inline struct adapter *rtw_netdev_priv(struct net_device *dev)
> +{
> + return (((struct rtw_netdev_priv_indicator *)netdev_priv(dev))->priv);
> +}
To be similar to existing uses, this variable name should be
netdev not dev. There are also unnecessary parentheses.
> void rtw_free_netdev(struct net_device *netdev);
Better to use netdev like this one.
---
static inline struct adapter *rtw_netdev_priv(struct net_device *netdev)
{
return ((struct rtw_netdev_priv_indicator *)netdev_priv(netdev))->priv;
}
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: android: ashmem: used const keyword
On Tue, 2020-07-28 at 23:29 +0530, Dhiraj Sharma wrote:
> I ran checkpatch.pl script which reported a warning to use const keyword
> on line 370.Therefore I made this change.
checkpatch is a brainless script.
Not everything it suggests is appropriate.
> diff --git a/drivers/staging/android/ashmem.c
> b/drivers/staging/android/ashmem.c
[]
> @@ -367,7 +367,7 @@ ashmem_vmfile_get_unmapped_area(struct file *file,
> unsigned long addr,
>
> static int ashmem_mmap(struct file *file, struct vm_area_struct *vma)
> {
> - static struct file_operations vmfile_fops;
> + static const struct file_operations vmfile_fops;
This can't work.
Please make sure to compile your proposed changes
_before_ you post them.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 3/4] staging: rtl8723bs: include: Further clean up function declarations
On Sun, 2020-07-26 at 17:02 +0200, Greg KH wrote: > On Sun, Jul 26, 2020 at 07:50:12PM +0530, Aditya Jain wrote: > > Cleaning up messy multiline function declarations in hal_phy_cfg.h [] > > diff --git a/drivers/staging/rtl8723bs/include/hal_phy_cfg.h > > b/drivers/staging/rtl8723bs/include/hal_phy_cfg.h [] > > -void > > -PHY_SetSwChnlBWMode8723B( > > -struct adapter *Adapter, > > -u8 channel, > > -enum CHANNEL_WIDTH Bandwidth, > > -u8 Offset40, > > -u8 Offset80 > > +void PHY_SetBWMode8723B(struct adapter *Adapter, > > + enum CHANNEL_WIDTH Bandwidth, /* 20M or 40M */ > > + unsigned char Offset/* Upper, Lower, or Don't care > > */ > > Those comments should go at the top of the function declaration, in > kernel doc format. Not every external function needs kernel-doc. This is a realtek staging driver that likely it will never be moved out of staging. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/1] STAGING - REALTEK RTL8188EU DRIVERS: Fix Coding Style Error
On Sat, 2020-07-25 at 12:47 -0500, Larry Finger wrote:
> On 7/25/20 7:20 AM, Anant Thazhemadam wrote:
> > Running the checkpatch.pl script on the file for which patch was created,
> > the
> > following error was found to exist.
> > ERROR: space required after that ',' (ctx:VxV)
> >
> > Fixed the above error which was found on line #721 by inserting a blank
> > space at the appropriate position.
[]
> > diff --git a/drivers/staging/rtl8188eu/core/rtw_security.c
> > b/drivers/staging/rtl8188eu/core/rtw_security.c
[]
> > @@ -718,7 +718,7 @@ u32 rtw_tkip_decrypt(struct adapter *padapter, u8
> > *precvframe)
> > res = _FAIL;
> > }
> > } else {
> > - RT_TRACE(_module_rtl871x_security_c_, _drv_err_, ("%s:
> > stainfo==NULL!!!\n",__func__));
> > + RT_TRACE(_module_rtl871x_security_c_, _drv_err_, ("%s:
> > stainfo==NULL!!!\n", __func__));
> > res = _FAIL;
> > }
> > }
>
> In fixing one checkpatch.pl condition, you introduced another - the resulting
> line is too long. You should fix only one such condition, but you should fix
> any
> others that are introduced. You do need to document it.
I think that doesn't matter as it was also too long
before this change.
> Patch subjects for this driver should be written as "staging: rtl8188eu:
> .".
How likely is it that this driver would ever be
moved to drivers/net/wireless/realtek/rtlwifi?
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: octeon: Indent with tabs instead of spaces
On Wed, 2020-07-22 at 22:19 +0500, Muhammad Usama Anjum wrote: > Remove a coding style error. It makes code more readable. [] > diff --git a/drivers/staging/octeon/ethernet-defines.h > b/drivers/staging/octeon/ethernet-defines.h [] > @@ -27,14 +27,14 @@ > #define REUSE_SKBUFFS_WITHOUT_FREE 1 > #endif > > -#define USE_ASYNC_IOBDMA(CONFIG_CAVIUM_OCTEON_CVMSEG_SIZE > 0) > +#define USE_ASYNC_IOBDMA (CONFIG_CAVIUM_OCTEON_CVMSEG_SIZE > 0) > > /* Maximum number of SKBs to try to free per xmit packet. */ > -#define MAX_OUT_QUEUE_DEPTH 1000 > +#define MAX_OUT_QUEUE_DEPTH 1000 > > #define FAU_TOTAL_TX_TO_CLEAN (CVMX_FAU_REG_END - sizeof(u32)) If you really like alignment to tabstop, why not align FAU_TOTAL_TX_TO_CLEAN too? ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: dpaa2-ethsw: fix switch/case fallthrough warning
On Mon, 2020-07-20 at 08:30 +0300, Marian Posteuca wrote:
> Fix the fallthrough warning that is reported by checkpatch.
>
> Signed-off-by: Marian Posteuca
> ---
> drivers/staging/fsl-dpaa2/ethsw/ethsw.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/fsl-dpaa2/ethsw/ethsw.c
> b/drivers/staging/fsl-dpaa2/ethsw/ethsw.c
> index 2fb75a7c9314..4986591097dc 100644
> --- a/drivers/staging/fsl-dpaa2/ethsw/ethsw.c
> +++ b/drivers/staging/fsl-dpaa2/ethsw/ethsw.c
> @@ -1362,7 +1362,8 @@ static int port_switchdev_blocking_event(struct
> notifier_block *unused,
> return NOTIFY_DONE;
>
> switch (event) {
> - case SWITCHDEV_PORT_OBJ_ADD: /* fall through */
> + case SWITCHDEV_PORT_OBJ_ADD:
> + fallthrough;
This comment is better removed instead of
converted to fallthrough;
It's no different than a multiple case block like:
switch (val) {
case FOO:
case BAR:
foobar();
break;
default:
return baz;
}
etc...
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 4/4] staging: rtl8188eu: include: placed constant on the right side of the test in comparisons
On Sat, 2020-07-18 at 05:18 -0400, B K Karthik wrote:
> placed constant on the right side of the test
> to fix warnings issued by checkpatch
[]
> diff --git a/drivers/staging/rtl8188eu/include/wifi.h
> b/drivers/staging/rtl8188eu/include/wifi.h
[]
> @@ -326,7 +326,7 @@ static inline unsigned char *get_hdr_bssid(unsigned char
> *pframe)
>
> static inline int IsFrameTypeCtrl(unsigned char *pframe)
> {
> - if (WIFI_CTRL_TYPE == GetFrameType(pframe))
> + if (GetFrameType(pframe) == WIFI_CTRL_TYPE)
> return true;
> else
> return false;
Always try to improve code instead of merely shutting
up checkpatch warnings.
This function should likely be written:
static inline bool IsFrameTypeCtrl(unsigned char *pframe)
{
return GetFrameType(pframe) == WIFI_CTRL_TYPE;
}
and given it's used only once, it might be expanded
in that place and removed altogether.
Something like:
(and the memcmp below could be ether_addr_equal instead
but I'm too lazy to find out if the addresses are both
guaranteed to be __aligned(2) which is likely)
---
drivers/staging/rtl8188eu/hal/rtl8188e_rxdesc.c | 8 +---
drivers/staging/rtl8188eu/include/wifi.h| 7 ---
2 files changed, 5 insertions(+), 10 deletions(-)
diff --git a/drivers/staging/rtl8188eu/hal/rtl8188e_rxdesc.c
b/drivers/staging/rtl8188eu/hal/rtl8188e_rxdesc.c
index 7d0135fde795..a2994f9ecbde 100644
--- a/drivers/staging/rtl8188eu/hal/rtl8188e_rxdesc.c
+++ b/drivers/staging/rtl8188eu/hal/rtl8188e_rxdesc.c
@@ -144,10 +144,12 @@ void update_recvframe_phyinfo_88e(struct recv_frame
*precvframe,
wlanhdr = precvframe->pkt->data;
- pkt_info.bPacketMatchBSSID = ((!IsFrameTypeCtrl(wlanhdr)) &&
- !pattrib->icv_err && !pattrib->crc_err &&
+ pkt_info.bPacketMatchBSSID =
+ GetFrameType(wlanhdr) != WIFI_CTRL_TYPE &&
+ !pattrib->icv_err &&
+ !pattrib->crc_err &&
!memcmp(get_hdr_bssid(wlanhdr),
-get_bssid(>mlmepriv), ETH_ALEN));
+ get_bssid(>mlmepriv), ETH_ALEN);
pkt_info.bPacketToSelf = pkt_info.bPacketMatchBSSID &&
(!memcmp(get_da(wlanhdr),
diff --git a/drivers/staging/rtl8188eu/include/wifi.h
b/drivers/staging/rtl8188eu/include/wifi.h
index 791f287a546d..3998d5633860 100644
--- a/drivers/staging/rtl8188eu/include/wifi.h
+++ b/drivers/staging/rtl8188eu/include/wifi.h
@@ -324,13 +324,6 @@ static inline unsigned char *get_hdr_bssid(unsigned char
*pframe)
return sa;
}
-static inline int IsFrameTypeCtrl(unsigned char *pframe)
-{
- if (WIFI_CTRL_TYPE == GetFrameType(pframe))
- return true;
- else
- return false;
-}
/*-
Below is for the security related definition
--*/
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: rtl*/security: Use static const in array declarations
Use static const in declarations where appropriate.
Signed-off-by: Joe Perches
---
drivers/staging/rtl8188eu/core/rtw_security.c | 4 ++--
drivers/staging/rtl8712/rtl871x_security.c| 2 +-
drivers/staging/rtl8723bs/core/rtw_security.c | 6 +++---
3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/rtl8188eu/core/rtw_security.c
b/drivers/staging/rtl8188eu/core/rtw_security.c
index f9139f72b3a7..3483c3d9cb56 100644
--- a/drivers/staging/rtl8188eu/core/rtw_security.c
+++ b/drivers/staging/rtl8188eu/core/rtw_security.c
@@ -734,7 +734,7 @@ u32 rtw_tkip_decrypt(struct adapter *padapter, u8
*precvframe)
/ SBOX Table */
/*/
-static u8 sbox_table[256] = {
+static const u8 sbox_table[256] = {
0x63, 0x7c, 0x77, 0x7b, 0xf2, 0x6b, 0x6f, 0xc5,
0x30, 0x01, 0x67, 0x2b, 0xfe, 0xd7, 0xab, 0x76,
0xca, 0x82, 0xc9, 0x7d, 0xfa, 0x59, 0x47, 0xf0,
@@ -817,7 +817,7 @@ static void next_key(u8 *key, int round)
{
u8 rcon;
u8 sbox_key[4];
- u8 rcon_table[12] = {
+ static const u8 rcon_table[12] = {
0x01, 0x02, 0x04, 0x08, 0x10, 0x20, 0x40, 0x80,
0x1b, 0x36, 0x36, 0x36
};
diff --git a/drivers/staging/rtl8712/rtl871x_security.c
b/drivers/staging/rtl8712/rtl871x_security.c
index 73e3d5ef3af2..c05010d85212 100644
--- a/drivers/staging/rtl8712/rtl871x_security.c
+++ b/drivers/staging/rtl8712/rtl871x_security.c
@@ -762,7 +762,7 @@ static void next_key(u8 *key, sint round)
{
u8 rcon;
u8 sbox_key[4];
- u8 rcon_table[12] = {
+ static const u8 rcon_table[12] = {
0x01, 0x02, 0x04, 0x08, 0x10, 0x20, 0x40, 0x80,
0x1b, 0x36, 0x36, 0x36
};
diff --git a/drivers/staging/rtl8723bs/core/rtw_security.c
b/drivers/staging/rtl8723bs/core/rtw_security.c
index ec9122070e58..7f74e1d05b3a 100644
--- a/drivers/staging/rtl8723bs/core/rtw_security.c
+++ b/drivers/staging/rtl8723bs/core/rtw_security.c
@@ -10,7 +10,7 @@
#include
#include
-static const char *_security_type_str[] = {
+static const char * const _security_type_str[] = {
"N/A",
"WEP40",
"TKIP",
@@ -842,7 +842,7 @@ u32 rtw_tkip_decrypt(struct adapter *padapter, u8
*precvframe)
/ SBOX Table */
/*/
- static u8 sbox_table[256] = {
+ static const u8 sbox_table[256] = {
0x63, 0x7c, 0x77, 0x7b, 0xf2, 0x6b, 0x6f, 0xc5,
0x30, 0x01, 0x67, 0x2b, 0xfe, 0xd7, 0xab, 0x76,
0xca, 0x82, 0xc9, 0x7d, 0xfa, 0x59, 0x47, 0xf0,
@@ -957,7 +957,7 @@ static void next_key(u8 *key, sint round)
{
u8 rcon;
u8 sbox_key[4];
- u8 rcon_table[12] = {
+ static const u8 rcon_table[12] = {
0x01, 0x02, 0x04, 0x08,
0x10, 0x20, 0x40, 0x80,
0x1b, 0x36, 0x36, 0x36
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] staging: gasket: core: Fix a coding style issue in gasket_core.c
On Wed, 2020-07-15 at 11:37 +0300, Dan Carpenter wrote: > On Wed, Jul 15, 2020 at 09:57:55AM +0200, Greg KH wrote: > > On Wed, Jul 15, 2020 at 12:24:22AM -0700, Joe Perches wrote: > > > On Wed, 2020-07-15 at 09:17 +0200, Greg KH wrote: > > > > On Wed, Jul 15, 2020 at 07:44:40AM +0800, Zhixu Zhao wrote: > > > > > On Thu, Jun 18, 2020 at 12:11:27AM +0800, Zhixu Zhao wrote: > > > > > > A coding alignment issue is found by checkpatch.pl. > > > > > > Fix it by using a temporary for gasket_dev->bar_data[bar_num]. > > > > > > > > > > > > Signed-off-by: Zhixu Zhao > > > > > > > > > > Hi, there~ > > > > > > > > > > Does anybody have any further comments on this? > > > > > Can it be merged? > > > > > > > > I never saw the first version of this, are you sure it got sent to the > > > > mailing list? It's not in any archives anywhere. > > > > > > I saw it. It's here: > > > https://lore.kernel.org/lkml/20200617161127.32006-1-zhixu...@126.com/ > > > > Ah, doh, sorry. > > > > Zhixu, please address the comments given to you on the series and resend > > it as a new version. > > He responded but not as a reply to my email. It turns out I made a > mistake. > > Anyway, just resend, Zhixu. It's a pity a resend is being requested. It'd be a better process if the original patch could be applied via the link akin to a git pull. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] staging: gasket: core: Fix a coding style issue in gasket_core.c
On Wed, 2020-07-15 at 09:17 +0200, Greg KH wrote: > On Wed, Jul 15, 2020 at 07:44:40AM +0800, Zhixu Zhao wrote: > > On Thu, Jun 18, 2020 at 12:11:27AM +0800, Zhixu Zhao wrote: > > > A coding alignment issue is found by checkpatch.pl. > > > Fix it by using a temporary for gasket_dev->bar_data[bar_num]. > > > > > > Signed-off-by: Zhixu Zhao > > > > Hi, there~ > > > > Does anybody have any further comments on this? > > Can it be merged? > > I never saw the first version of this, are you sure it got sent to the > mailing list? It's not in any archives anywhere. I saw it. It's here: https://lore.kernel.org/lkml/20200617161127.32006-1-zhixu...@126.com/ > Also, 3 days is really fast for a simple coding style cleanup to be > worried about. Give it usually at least 2 weeks. Originally sent June 17. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 6/6] staging: qlge: qlge_ethtool: Remove one byte memset.
On Wed, 2020-07-15 at 00:36 +0530, Suraj Upadhyay wrote: > On Tue, Jul 14, 2020 at 11:57:23AM -0700, Joe Perches wrote: > > On Mon, 2020-07-13 at 17:17 +0300, Dan Carpenter wrote: > > > On Mon, Jul 13, 2020 at 05:52:22PM +0530, Suraj Upadhyay wrote: > > > > Use direct assignment instead of using memset with just one byte as an > > > > argument. > > > > Issue found by checkpatch.pl. > > > > > > > > Signed-off-by: Suraj Upadhyay > > > > --- > > > > Hii Maintainers, > > > > Please correct me if I am wrong here. > > > > --- > > > > > > > > drivers/staging/qlge/qlge_ethtool.c | 4 ++-- > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/staging/qlge/qlge_ethtool.c > > > > b/drivers/staging/qlge/qlge_ethtool.c > > > > index 16fcdefa9687..d44b2dae9213 100644 > > > > --- a/drivers/staging/qlge/qlge_ethtool.c > > > > +++ b/drivers/staging/qlge/qlge_ethtool.c > > > > @@ -516,8 +516,8 @@ static void ql_create_lb_frame(struct sk_buff *skb, > > > > memset(skb->data, 0xFF, frame_size); > > > > frame_size &= ~1; > > > > memset(>data[frame_size / 2], 0xAA, frame_size / 2 - 1); > > > > - memset(>data[frame_size / 2 + 10], 0xBE, 1); > > > > - memset(>data[frame_size / 2 + 12], 0xAF, 1); > > > > + skb->data[frame_size / 2 + 10] = (unsigned char)0xBE; > > > > + skb->data[frame_size / 2 + 12] = (unsigned char)0xAF; > > > > > > Remove the casting. > > > > > > I guess this is better than the original because now it looks like > > > ql_check_lb_frame(). It's still really weird looking though. > > > > There are several of these in the intel drivers too: > > > > drivers/net/ethernet/intel/e1000/e1000_ethtool.c: > > memset(>data[frame_size / 2 + 10], 0xBE, 1); > > drivers/net/ethernet/intel/e1000/e1000_ethtool.c: > > memset(>data[frame_size / 2 + 12], 0xAF, 1); > > drivers/net/ethernet/intel/e1000e/ethtool.c: > > memset(>data[frame_size / 2 + 10], 0xBE, 1); > > drivers/net/ethernet/intel/e1000e/ethtool.c: > > memset(>data[frame_size / 2 + 12], 0xAF, 1); > > drivers/net/ethernet/intel/igb/igb_ethtool.c: > > memset(>data[frame_size + 10], 0xBE, 1); > > drivers/net/ethernet/intel/igb/igb_ethtool.c: > > memset(>data[frame_size + 12], 0xAF, 1); > > drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c: > > memset(>data[frame_size + 10], 0xBE, 1); > > drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c: > > memset(>data[frame_size + 12], 0xAF, 1); > > drivers/staging/qlge/qlge_ethtool.c:memset(>data[frame_size / 2 + > > 10], 0xBE, 1); > > drivers/staging/qlge/qlge_ethtool.c:memset(>data[frame_size / 2 + > > 12], 0xAF, 1); > > Thanks to point this out, > I will be sending a patchset for that soon. It _might_ be useful to create and use a standard mechanism for the loopback functions: create_lbtest_frame and check_lbtest_frame Maybe use something like: ether_loopback_frame_create and ether_loopback_frame_check ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 6/6] staging: qlge: qlge_ethtool: Remove one byte memset.
On Mon, 2020-07-13 at 17:17 +0300, Dan Carpenter wrote: > On Mon, Jul 13, 2020 at 05:52:22PM +0530, Suraj Upadhyay wrote: > > Use direct assignment instead of using memset with just one byte as an > > argument. > > Issue found by checkpatch.pl. > > > > Signed-off-by: Suraj Upadhyay > > --- > > Hii Maintainers, > > Please correct me if I am wrong here. > > --- > > > > drivers/staging/qlge/qlge_ethtool.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/staging/qlge/qlge_ethtool.c > > b/drivers/staging/qlge/qlge_ethtool.c > > index 16fcdefa9687..d44b2dae9213 100644 > > --- a/drivers/staging/qlge/qlge_ethtool.c > > +++ b/drivers/staging/qlge/qlge_ethtool.c > > @@ -516,8 +516,8 @@ static void ql_create_lb_frame(struct sk_buff *skb, > > memset(skb->data, 0xFF, frame_size); > > frame_size &= ~1; > > memset(>data[frame_size / 2], 0xAA, frame_size / 2 - 1); > > - memset(>data[frame_size / 2 + 10], 0xBE, 1); > > - memset(>data[frame_size / 2 + 12], 0xAF, 1); > > + skb->data[frame_size / 2 + 10] = (unsigned char)0xBE; > > + skb->data[frame_size / 2 + 12] = (unsigned char)0xAF; > > Remove the casting. > > I guess this is better than the original because now it looks like > ql_check_lb_frame(). It's still really weird looking though. There are several of these in the intel drivers too: drivers/net/ethernet/intel/e1000/e1000_ethtool.c: memset(>data[frame_size / 2 + 10], 0xBE, 1); drivers/net/ethernet/intel/e1000/e1000_ethtool.c: memset(>data[frame_size / 2 + 12], 0xAF, 1); drivers/net/ethernet/intel/e1000e/ethtool.c:memset(>data[frame_size / 2 + 10], 0xBE, 1); drivers/net/ethernet/intel/e1000e/ethtool.c:memset(>data[frame_size / 2 + 12], 0xAF, 1); drivers/net/ethernet/intel/igb/igb_ethtool.c: memset(>data[frame_size + 10], 0xBE, 1); drivers/net/ethernet/intel/igb/igb_ethtool.c: memset(>data[frame_size + 12], 0xAF, 1); drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c: memset(>data[frame_size + 10], 0xBE, 1); drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c: memset(>data[frame_size + 12], 0xAF, 1); drivers/staging/qlge/qlge_ethtool.c:memset(>data[frame_size / 2 + 10], 0xBE, 1); drivers/staging/qlge/qlge_ethtool.c:memset(>data[frame_size / 2 + 12], 0xAF, 1); ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: media: usbvision: removing prohibited space before ',' (ctx:WxW)
On Sat, 2020-06-27 at 10:49 +0200, Greg Kroah-Hartman wrote: > On Sat, Jun 27, 2020 at 10:28:31AM +0200, Hans Verkuil wrote: > > On 27/06/2020 07:07, Greg Kroah-Hartman wrote: > > > A: http://en.wikipedia.org/wiki/Top_post > > > Q: Were do I find info about this thing called top-posting? > > > A: Because it messes up the order in which people normally read text. > > > Q: Why is top-posting such a bad thing? > > > A: Top-posting. > > > Q: What is the most annoying thing in e-mail? > > > > > > A: No. > > > Q: Should I include quotations after my reply? > > > > > > http://daringfireball.net/2007/07/on_top > > > > > > On Fri, Jun 26, 2020 at 11:42:49AM -0400, B K KARTHIK > > > PES2201800185STUDENT ECE DeptPESU EC Campus wrote: > > > > Oh, I'm sorry but wouldn't it be helpful if we had a file that lists > > > > all drivers that are scheduled for removal? > > > > > > The TODO file in the directory for the driver should have this > > > information in it. I don't know if all of the media drivers have this, > > > if not, then there is no way you could have known this. > > > > They have, and in addition the Kconfig entry will mention that the driver > > is deprecated. > > > > TODO of usbvision: > > > > The driver is deprecated and scheduled for removal by the end > > of 2020. > > > > In order to prevent removal the following actions would have to > > be taken: > > > > - clean up the code > > - convert to the vb2 framework > > - fix the disconnect and free-on-last-user handling (i.e., add > > a release callback for struct v4l2_device and rework the code > > to use that correctly). > > Ah, great, nevermind then! > > B K, your wish is already granted, the text is present, you just needed > to have noticed it :) > > greg k-h You should mark the entry in MAINTAINERS as obsolete so checkpatch tells people not to send patches. --- MAINTAINERS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/MAINTAINERS b/MAINTAINERS index 04fceaee5200..7c136018d153 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -17906,7 +17906,7 @@ F: include/uapi/linux/uvcvideo.h USB VISION DRIVER M: Hans Verkuil L: linux-me...@vger.kernel.org -S: Odd Fixes +S: Odd Fixes / Obsolete W: https://linuxtv.org T: git git://linuxtv.org/media_tree.git F: drivers/staging/media/usbvision/ ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: RT_TRACE in drivers/staging/rtl8188eu
On Sat, 2020-06-27 at 10:18 +0200, Greg KH wrote: > On Sat, Jun 27, 2020 at 01:12:42AM -0700, Joe Perches wrote: > > On Sat, 2020-06-27 at 10:01 +0200, Greg KH wrote: > > > On Sat, Jun 27, 2020 at 12:33:56AM -0700, Joe Perches wrote: > > > > There are 3 parts of the email. > > > > > > > > 1: A description and patch for a logging defect > > > > 2: A script to go along with the patch to do conversions > > > > 3: Current diff for this defect > > > > > > > > > > > > - > > > > > > > > The macro below in drivers/staging/rtl8188eu/include/rtw_debug.h > > > > is defective as it emits multiple pr_info calls for each use > > > > so the logging in dmesg is discontinuous. > > > > > > I recommend just deleting it. As it's obviously incorrect, and any > > > "real" tracing should just use the real tracing infrastructure, this is > > > not needed and can be removed. > > > > Don't get hung up on the name. > > > > It's not used for tracing, it's effectively just > > a debugging mechanism, the same in all the other > > rtl staging drivers. > > Ok, then it should be converted to "normal" dev_*() functions, where > needed, the others deleted entirely (the dev_info attempts...) Nope. These are the same as uses in drivers/net/wireless/realtek/rtlwifi. $ git grep -w RT_TRACE drivers/net/wireless/realtek | wc -l 2847 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: RT_TRACE in drivers/staging/rtl8188eu
On Sat, 2020-06-27 at 10:01 +0200, Greg KH wrote: > On Sat, Jun 27, 2020 at 12:33:56AM -0700, Joe Perches wrote: > > There are 3 parts of the email. > > > > 1: A description and patch for a logging defect > > 2: A script to go along with the patch to do conversions > > 3: Current diff for this defect > > > > > > - > > > > The macro below in drivers/staging/rtl8188eu/include/rtw_debug.h > > is defective as it emits multiple pr_info calls for each use > > so the logging in dmesg is discontinuous. > > I recommend just deleting it. As it's obviously incorrect, and any > "real" tracing should just use the real tracing infrastructure, this is > not needed and can be removed. Don't get hung up on the name. It's not used for tracing, it's effectively just a debugging mechanism, the same in all the other rtl staging drivers. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] fbtft-bus.c: Removing that prohibited space before ')'
On Sat, 2020-06-27 at 00:51 -0400, B K Karthik wrote: > fbtft-bus.c: > > fixing ERROR: space prohibited before that close parenthesis ')' by removing > that space and ',' in line 65 and 67. [] > diff --git a/drivers/staging/fbtft/fbtft-bus.c > b/drivers/staging/fbtft/fbtft-bus.c > index 63c65dd67b17..847cbfbbd766 100644 > --- a/drivers/staging/fbtft/fbtft-bus.c > +++ b/drivers/staging/fbtft/fbtft-bus.c > @@ -62,9 +62,9 @@ out: > \ > } > \ > EXPORT_SYMBOL(func); > > -define_fbtft_write_reg(fbtft_write_reg8_bus8, u8, u8, ) > +define_fbtft_write_reg(fbtft_write_reg8_bus8, u8, u8) > define_fbtft_write_reg(fbtft_write_reg16_bus8, __be16, u16, cpu_to_be16) > -define_fbtft_write_reg(fbtft_write_reg16_bus16, u16, u16, ) > +define_fbtft_write_reg(fbtft_write_reg16_bus16, u16, u16) Q: Did you compile the files modified by this patch before you submitted it? A: No ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/2] staging: qlge: fix else after return or break
On Sat, 2020-06-27 at 07:57 +0800, Coiby Xu wrote: > On Thu, Jun 25, 2020 at 03:13:14PM -0700, Joe Perches wrote: > > On Fri, 2020-06-26 at 05:57 +0800, Coiby Xu wrote: > > > Remove unnecessary elses after return or break. > > > > unrelated trivia: [] > > looks like all of these could use netdev_err [] > should we also replace all pr_errs with netdev_err in > ql_dump_* functions? Ideally, anywhere a struct netdevice * is available, it should be used to output netdev_ in preference to pr_. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/2] staging: qlge: fix else after return or break
On Fri, 2020-06-26 at 05:57 +0800, Coiby Xu wrote:
> Remove unnecessary elses after return or break.
unrelated trivia:
> diff --git a/drivers/staging/qlge/qlge_dbg.c b/drivers/staging/qlge/qlge_dbg.c
[]
> @@ -1391,12 +1391,11 @@ static void ql_dump_cam_entries(struct ql_adapter
> *qdev)
> pr_err("%s: Failed read of mac index register\n",
> __func__);
> return;
> - } else {
> - if (value[0])
> - pr_err("%s: CAM index %d CAM Lookup Lower =
> 0x%.08x:%.08x, Output = 0x%.08x\n",
> -qdev->ndev->name, i, value[1], value[0],
> -value[2]);
looks like all of these could use netdev_err
netdev_err(qdev, "etc...",
i, value[1], value[0], value[2]);
etc...
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 03/50] staging: mmal-vchiq: Avoid use of bool in structures
On Tue, 2020-06-23 at 18:41 +0200, Nicolas Saenz Julienne wrote: > From: Dave Stevenson > > Fixes up a checkpatch error "Avoid using bool structure members > because of possible alignment issues". [] > diff --git a/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c > b/drivers/staging/vc04_services/vchiq-mmal/mmal-vchiq.c [] > @@ -1754,7 +1754,7 @@ int vchiq_mmal_component_enable(struct > vchiq_mmal_instance *instance, > > ret = enable_component(instance, component); > if (ret == 0) > - component->enabled = true; > + component->enabled = 1; This change does not match the commit description. Also, checkpatch does not emit a warning here. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 0/3] mm, treewide: Rename kzfree() to kfree_sensitive()
On Thu, 2020-06-18 at 00:31 +0300, Denis Efremov wrote: > > On 6/16/20 9:53 PM, Joe Perches wrote: > > On Mon, 2020-06-15 at 21:57 -0400, Waiman Long wrote: > > > v4: > > > - Break out the memzero_explicit() change as suggested by Dan Carpenter > > > so that it can be backported to stable. > > > - Drop the "crypto: Remove unnecessary memzero_explicit()" patch for > > > now as there can be a bit more discussion on what is best. It will be > > > introduced as a separate patch later on after this one is merged. > > > > To this larger audience and last week without reply: > > https://lore.kernel.org/lkml/573b3fbd5927c643920e1364230c296b23e7584d.ca...@perches.com/ > > > > Are there _any_ fastpath uses of kfree or vfree? > > > > Many patches have been posted recently to fix mispairings > > of specific types of alloc and free functions. > > I've prepared a coccinelle script to highlight these mispairings in a function > a couple of days ago: https://lkml.org/lkml/2020/6/5/953 > I've listed all the fixes in the commit message. > > Not so many mispairings actually, and most of them are harmless like: > kmalloc(E) -> kvfree(E) > > However, coccinelle script can't detect cross-functions mispairings, i.e. > allocation in one function, free in another funtion. Hey Denis, thanks for those patches. If possible, it's probably better to not require these pairings and use a single standard kfree/free function. Given the existing ifs in kfree in slab/slob/slub, it seems likely that adding a few more wouldn't have much impact. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 0/3] mm, treewide: Rename kzfree() to kfree_sensitive()
On Mon, 2020-06-15 at 21:57 -0400, Waiman Long wrote:
> v4:
> - Break out the memzero_explicit() change as suggested by Dan Carpenter
> so that it can be backported to stable.
> - Drop the "crypto: Remove unnecessary memzero_explicit()" patch for
> now as there can be a bit more discussion on what is best. It will be
> introduced as a separate patch later on after this one is merged.
To this larger audience and last week without reply:
https://lore.kernel.org/lkml/573b3fbd5927c643920e1364230c296b23e7584d.ca...@perches.com/
Are there _any_ fastpath uses of kfree or vfree?
Many patches have been posted recently to fix mispairings
of specific types of alloc and free functions.
To eliminate these mispairings at a runtime cost of four
comparisons, should the kfree/vfree/kvfree/kfree_const
functions be consolidated into a single kfree?
Something like the below:
void kfree(const void *addr)
{
if (is_kernel_rodata((unsigned long)addr))
return;
if (is_vmalloc_addr(addr))
_vfree(addr);
else
_kfree(addr);
}
#define kvfree kfree
#define vfreekfree
#define kfree_const kfree
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: gasket: core: Fix a coding style issue in gasket_core.c
On Sun, 2020-06-14 at 21:51 +0800, Zhixu Zhao wrote:
> Fix a coding alignment issue found by checkpatch.pl.
Another option would be to use a temporary for
gasket_dev->bar_data[bar_num]
Something like:
---
drivers/staging/gasket/gasket_core.c | 29 ++---
1 file changed, 14 insertions(+), 15 deletions(-)
diff --git a/drivers/staging/gasket/gasket_core.c
b/drivers/staging/gasket/gasket_core.c
index 67325fbaf760..73b138f984cf 100644
--- a/drivers/staging/gasket/gasket_core.c
+++ b/drivers/staging/gasket/gasket_core.c
@@ -262,6 +262,7 @@ static int gasket_map_pci_bar(struct gasket_dev
*gasket_dev, int bar_num)
internal_desc->driver_desc;
ulong desc_bytes = driver_desc->bar_descriptions[bar_num].size;
int ret;
+ struct gasket_bar_data *data;
if (desc_bytes == 0)
return 0;
@@ -270,31 +271,32 @@ static int gasket_map_pci_bar(struct gasket_dev
*gasket_dev, int bar_num)
/* not PCI: skip this entry */
return 0;
}
+
+ data = _dev->bar_data[bar_num];
+
/*
* pci_resource_start and pci_resource_len return a "resource_size_t",
* which is safely castable to ulong (which itself is the arg to
* request_mem_region).
*/
- gasket_dev->bar_data[bar_num].phys_base =
+ data->phys_base =
(ulong)pci_resource_start(gasket_dev->pci_dev, bar_num);
- if (!gasket_dev->bar_data[bar_num].phys_base) {
+ if (!data->phys_base) {
dev_err(gasket_dev->dev, "Cannot get BAR%u base address\n",
bar_num);
return -EINVAL;
}
- gasket_dev->bar_data[bar_num].length_bytes =
+ data->length_bytes =
(ulong)pci_resource_len(gasket_dev->pci_dev, bar_num);
- if (gasket_dev->bar_data[bar_num].length_bytes < desc_bytes) {
+ if (data->length_bytes < desc_bytes) {
dev_err(gasket_dev->dev,
"PCI BAR %u space is too small: %lu; expected >= %lu\n",
- bar_num, gasket_dev->bar_data[bar_num].length_bytes,
- desc_bytes);
+ bar_num, data->length_bytes, desc_bytes);
return -ENOMEM;
}
- if (!request_mem_region(gasket_dev->bar_data[bar_num].phys_base,
- gasket_dev->bar_data[bar_num].length_bytes,
+ if (!request_mem_region(data->phys_base, data->length_bytes,
gasket_dev->dev_info.name)) {
dev_err(gasket_dev->dev,
"Cannot get BAR %d memory region %p\n",
@@ -302,10 +304,8 @@ static int gasket_map_pci_bar(struct gasket_dev
*gasket_dev, int bar_num)
return -EINVAL;
}
- gasket_dev->bar_data[bar_num].virt_base =
- ioremap(gasket_dev->bar_data[bar_num].phys_base,
- gasket_dev->bar_data[bar_num].length_bytes);
- if (!gasket_dev->bar_data[bar_num].virt_base) {
+ data->virt_base = ioremap(data->phys_base, data->length_bytes);
+ if (!data->virt_base) {
dev_err(gasket_dev->dev,
"Cannot remap BAR %d memory region %p\n",
bar_num, _dev->pci_dev->resource[bar_num]);
@@ -319,9 +319,8 @@ static int gasket_map_pci_bar(struct gasket_dev
*gasket_dev, int bar_num)
return 0;
fail:
- iounmap(gasket_dev->bar_data[bar_num].virt_base);
- release_mem_region(gasket_dev->bar_data[bar_num].phys_base,
- gasket_dev->bar_data[bar_num].length_bytes);
+ iounmap(data->virt_base);
+ release_mem_region(data->phys_base, data->length_bytes);
return ret;
}
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/2] media: atomisp: replace old ---help--- tags by just help
On Mon, 2020-06-15 at 07:22 +0200, Mauro Carvalho Chehab wrote:
> Em Mon, 15 Jun 2020 07:18:34 +0200
> Mauro Carvalho Chehab escreveu:
>
> > There are several places on this file using the deprecated
> > ---help--- tag.
> >
> > Replaces them.
>
> Just ignore it. A treewide patch already changed it:
>
> a7f7f6248d97 ("treewide: replace '---help---' in Kconfig files with
> 'help'")
>
> I'll double-check the spacing things here. If it lasts upstream, I'll
> send the changes on a separate patch.
Another possible change to avoid more ---help--- lines:
---
scripts/checkkconfigsymbols.py | 2 +-
scripts/checkpatch.pl | 6 +-
scripts/kconfig/lexer.l| 2 +-
3 files changed, 3 insertions(+), 7 deletions(-)
diff --git a/scripts/checkkconfigsymbols.py b/scripts/checkkconfigsymbols.py
index 00a10a293f4f..1548f9ce4682 100755
--- a/scripts/checkkconfigsymbols.py
+++ b/scripts/checkkconfigsymbols.py
@@ -34,7 +34,7 @@ REGEX_SOURCE_SYMBOL = re.compile(SOURCE_SYMBOL)
REGEX_KCONFIG_DEF = re.compile(DEF)
REGEX_KCONFIG_EXPR = re.compile(EXPR)
REGEX_KCONFIG_STMT = re.compile(STMT)
-REGEX_KCONFIG_HELP = re.compile(r"^\s+(help|---help---)\s*$")
+REGEX_KCONFIG_HELP = re.compile(r"^\s+help\s*$")
REGEX_FILTER_SYMBOLS = re.compile(r"[A-Za-z0-9]$")
REGEX_NUMERIC = re.compile(r"0[xX][0-9a-fA-F]+|[0-9]+")
REGEX_QUOTES = re.compile("(\"(.*?)\")")
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 524df88f9364..738bb3fcf202 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3044,11 +3044,7 @@ sub process {
if ($lines[$ln - 1] =~
/^\+\s*(?:bool|tristate|prompt)\s*["']/) {
$is_start = 1;
- } elsif ($lines[$ln - 1] =~
/^\+\s*(?:help|---help---)\s*$/) {
- if ($lines[$ln - 1] =~ "---help---") {
- WARN("CONFIG_DESCRIPTION",
-"prefer 'help' over
'---help---' for new help texts\n" . $herecurr);
- }
+ } elsif ($lines[$ln - 1] =~ /^\+\s*help\s*$/) {
$length = -1;
}
diff --git a/scripts/kconfig/lexer.l b/scripts/kconfig/lexer.l
index 6354c905b006..4b7339ff4c8b 100644
--- a/scripts/kconfig/lexer.l
+++ b/scripts/kconfig/lexer.l
@@ -105,7 +105,7 @@ n [A-Za-z0-9_-]
"endchoice"return T_ENDCHOICE;
"endif"return T_ENDIF;
"endmenu" return T_ENDMENU;
-"help"|"---help---"return T_HELP;
+"help" return T_HELP;
"hex" return T_HEX;
"if" return T_IF;
"imply"return T_IMPLY;
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Forest Bond ,Greg Kroah-Hartman , de...@driverdev.osuosl.org, linux-ker...@vger.kernel.org
On Mon, 2020-06-08 at 22:58 +, Rodolfo C Villordo wrote: > On Mon, Jun 08, 2020 at 01:41:11AM -0700, Joe Perches wrote: > > On Mon, 2020-06-08 at 07:59 +0200, Julia Lawall wrote: > > > On Mon, 8 Jun 2020, Al Viro wrote: > > > > > > > On Sun, Jun 07, 2020 at 10:41:56PM +, Rodolfo C. Villordo wrote: > > > > > Multiple line over 80 characters fixes by splitting in multiple lines. > > > > > Warning found by checkpatch.pl > > > > > > > > I doubt that checkpatch.pl can catch the real problems there: > > > > > > > > * Hungarian Notation Sucks. Really. > > > > * so does CamelCase, especially for wonders like s_uGetRTSCTSRsvTime > > Yes, I agree with that. > > > > Rodolfo, > > > > > > If you work hard with Coccinelle and python scripting, it can help with > > > the first two problems. > > > > These VIA vt6655/vt6656 drivers have been in staging for more than > > a decade. There are relatively few checkpatch coding style > > cleanups to do but there are many overall style issues to resolve. > > > > Yes, vt6655/rxtx.c needs lots of work. I was avoiding submit bigger changes > because this is my second patch submission. > > Thank you all for the comments. I'm really sorry for the odd subject. > > How should I move forward with this? > > 1 - Update this patch with the changes pointed by Dan Carpenter? Keep your changes small until you really know how this style of linux kernel staging changes is done. > 2 - Do a more elaborated and bigger change, like suggested by Al Viro > and Joe Perches? A patch series is much preferred to a single large change. If you decide to refactor various functions, please do that in separate, discrete patches. Adding a #define and doing a sed like: $ sed -i 's/(BY_AL2230_REG_LEN << 3) + IFREGCTL_REGW/AL2230_RLEN_CTL/' drivers/staging/vt6655/*.[ch] should be a single patch. And if you do that, another should be done for AL7230 $ sed -i 's/(BY_AL7230_REG_LEN << 3) + IFREGCTL_REGW/AL7230_RLEN_CTL/' drivers/staging/vt6655/*.[ch] etc... Maybe the #define BY_AL2230_REG_LEN should be 0x17 so that the << 3 is more obviously constrained to the low byte Maybe the + uses in the macros should be bitwise |. Go wild after you figure out the process, just keep your patches to obvious, small and verifiable changes. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging:r8723bs: remove wrappers around skb_clone()
On Sun, 2020-05-31 at 20:28 +0300, Ivan Safonov wrote:
> On 5/31/20 7:15 PM, Joe Perches wrote:
> > On Sun, 2020-05-31 at 19:08 +0300, Ivan Safonov wrote:
> > > Wrappers around skb_clone() do not simplify the driver code.
> > []
> > > -inline struct sk_buff *_rtw_skb_clone(struct sk_buff *skb)
> > > -{
> > > - return skb_clone(skb, in_interrupt() ? GFP_ATOMIC : GFP_KERNEL);
> > > -}
> > > -
> > []
> > > diff --git a/drivers/staging/rtl8723bs/os_dep/recv_linux.c
> > > b/drivers/staging/rtl8723bs/os_dep/recv_linux.c
> > []
> > > @@ -110,7 +110,7 @@ void rtw_os_recv_indicate_pkt(struct adapter
> > > *padapter, _pkt *pkt, struct rx_pkt
> > > if (memcmp(pattrib->dst,
> > > myid(>eeprompriv), ETH_ALEN)) {
> > > if (bmcast) {
> > > psta =
> > > rtw_get_bcmc_stainfo(padapter);
> > > - pskb2 = rtw_skb_clone(pkt);
> > > + pskb2 = skb_clone(pkt, GFP_ATOMIC);
> >
> > Why make every clone allocation GFP_ATOMIC ?
>
> The rtw_os_recv_indicate_pkt() is always called from an interrupt handler.
It'd be better to indicate you know that in the changelog
as the subject and changelog just shows removing wrappers
and the patch code does not agree with that.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging:r8723bs: remove wrappers around skb_clone()
On Sun, 2020-05-31 at 19:08 +0300, Ivan Safonov wrote:
> Wrappers around skb_clone() do not simplify the driver code.
[]
> -inline struct sk_buff *_rtw_skb_clone(struct sk_buff *skb)
> -{
> - return skb_clone(skb, in_interrupt() ? GFP_ATOMIC : GFP_KERNEL);
> -}
> -
[]
> diff --git a/drivers/staging/rtl8723bs/os_dep/recv_linux.c
> b/drivers/staging/rtl8723bs/os_dep/recv_linux.c
[]
> @@ -110,7 +110,7 @@ void rtw_os_recv_indicate_pkt(struct adapter *padapter,
> _pkt *pkt, struct rx_pkt
> if (memcmp(pattrib->dst, myid(>eeprompriv),
> ETH_ALEN)) {
> if (bmcast) {
> psta = rtw_get_bcmc_stainfo(padapter);
> - pskb2 = rtw_skb_clone(pkt);
> + pskb2 = skb_clone(pkt, GFP_ATOMIC);
Why make every clone allocation GFP_ATOMIC ?
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 2/2] staging: rtl8188eu: make some arrays static const
On Sun, 2020-05-24 at 12:15 +0200, Michael Straube wrote: > Make some arrays in phy_iq_calibrate() static const and adjust > the functions that take these arrays as parameters accordingly. > Reduces object file size by 84 bytes (GCC 9.3.1 x86_64). You could also do drivers/net/wireless/realtek/rtlwifi/rtl8192de/phy.c and reduce data by a fair amount there too. Just change the prototype for _rtl92d_phy_calc_curvindex there. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rtl8188eu: clean up some declarations
On Sun, 2020-05-24 at 10:27 +0200, Michael Straube wrote:
> Clean up some array declarations in phy_iq_calibrate() to reduce
> indentation and clear line over 80 characters checkpatch warnings.
Better still would be to mark these as static const
and also mark the functions that use them to take
const pointers
Something like:
---
drivers/staging/rtl8188eu/hal/phy.c | 52 -
1 file changed, 28 insertions(+), 24 deletions(-)
diff --git a/drivers/staging/rtl8188eu/hal/phy.c
b/drivers/staging/rtl8188eu/hal/phy.c
index 5eca3625d5a8..9a4f1b84fefc 100644
--- a/drivers/staging/rtl8188eu/hal/phy.c
+++ b/drivers/staging/rtl8188eu/hal/phy.c
@@ -786,7 +786,7 @@ static void pathb_fill_iqk(struct adapter *adapt, bool
iqkok, s32 result[][8],
}
}
-static void save_adda_registers(struct adapter *adapt, u32 *addareg,
+static void save_adda_registers(struct adapter *adapt, const u32 *addareg,
u32 *backup, u32 register_num)
{
u32 i;
@@ -795,7 +795,7 @@ static void save_adda_registers(struct adapter *adapt, u32
*addareg,
backup[i] = phy_query_bb_reg(adapt, addareg[i], bMaskDWord);
}
-static void save_mac_registers(struct adapter *adapt, u32 *mac_reg,
+static void save_mac_registers(struct adapter *adapt, const u32 *mac_reg,
u32 *backup)
{
u32 i;
@@ -806,7 +806,7 @@ static void save_mac_registers(struct adapter *adapt, u32
*mac_reg,
backup[i] = usb_read32(adapt, mac_reg[i]);
}
-static void reload_adda_reg(struct adapter *adapt, u32 *adda_reg,
+static void reload_adda_reg(struct adapter *adapt, const u32 *adda_reg,
u32 *backup, u32 regiester_num)
{
u32 i;
@@ -816,7 +816,7 @@ static void reload_adda_reg(struct adapter *adapt, u32
*adda_reg,
}
static void reload_mac_registers(struct adapter *adapt,
-u32 *mac_reg, u32 *backup)
+const u32 *mac_reg, u32 *backup)
{
u32 i;
@@ -826,7 +826,7 @@ static void reload_mac_registers(struct adapter *adapt,
usb_write32(adapt, mac_reg[i], backup[i]);
}
-static void path_adda_on(struct adapter *adapt, u32 *adda_reg,
+static void path_adda_on(struct adapter *adapt, const u32 *adda_reg,
bool is_path_a_on, bool is2t)
{
u32 path_on;
@@ -844,7 +844,8 @@ static void path_adda_on(struct adapter *adapt, u32
*adda_reg,
phy_set_bb_reg(adapt, adda_reg[i], bMaskDWord, path_on);
}
-static void mac_setting_calibration(struct adapter *adapt, u32 *mac_reg, u32
*backup)
+static void mac_setting_calibration(struct adapter *adapt, const u32 *mac_reg,
+ u32 *backup)
{
u32 i = 0;
@@ -952,26 +953,29 @@ static void phy_iq_calibrate(struct adapter *adapt, s32
result[][8],
struct odm_dm_struct *dm_odm = >HalData->odmpriv;
u32 i;
u8 path_a_ok, path_b_ok;
- u32 adda_reg[IQK_ADDA_REG_NUM] = {
- rFPGA0_XCD_SwitchControl, rBlue_Tooth,
- rRx_Wait_CCA, rTx_CCK_RFON,
- rTx_CCK_BBON, rTx_OFDM_RFON,
- rTx_OFDM_BBON, rTx_To_Rx,
- rTx_To_Tx, rRx_CCK,
- rRx_OFDM, rRx_Wait_RIFS,
- rRx_TO_Rx, rStandby,
- rSleep, rPMPD_ANAEN};
-
- u32 iqk_mac_reg[IQK_MAC_REG_NUM] = {
- REG_TXPAUSE, REG_BCN_CTRL,
- REG_BCN_CTRL_1, REG_GPIO_MUXCFG};
+ static const u32 adda_reg[IQK_ADDA_REG_NUM] = {
+ rFPGA0_XCD_SwitchControl, rBlue_Tooth,
+ rRx_Wait_CCA, rTx_CCK_RFON,
+ rTx_CCK_BBON, rTx_OFDM_RFON,
+ rTx_OFDM_BBON, rTx_To_Rx,
+ rTx_To_Tx, rRx_CCK,
+ rRx_OFDM, rRx_Wait_RIFS,
+ rRx_TO_Rx, rStandby,
+ rSleep, rPMPD_ANAEN
+ };
+
+ static const u32 iqk_mac_reg[IQK_MAC_REG_NUM] = {
+ REG_TXPAUSE, REG_BCN_CTRL,
+ REG_BCN_CTRL_1, REG_GPIO_MUXCFG
+ };
/* since 92C & 92D have the different define in IQK_BB_REG */
- u32 iqk_bb_reg_92c[IQK_BB_REG_NUM] = {
- rOFDM0_TRxPathEnable,
rOFDM0_TRMuxPar,
- rFPGA0_XCD_RFInterfaceSW,
rConfig_AntA, rConfig_AntB,
- rFPGA0_XAB_RFInterfaceSW,
rFPGA0_XA_RFInterfaceOE,
- rFPGA0_XB_RFInterfaceOE,
rFPGA0_RFMOD};
+ static const u32 iqk_bb_reg_92c[IQK_BB_REG_NUM] = {
+ rOFDM0_TRxPathEnable, rOFDM0_TRMuxPar,
+ rFPGA0_XCD_RFInterfaceSW,
Re: [PATCH] taging: speakup: remove volatile
On Fri, 2020-05-22 at 13:34 +0300, Dan Carpenter wrote: > On Fri, May 22, 2020 at 02:46:28PM +0530, MugilRaj wrote: > > fix checkpatch.pl warning, which is Use of volatile is usually wrong: see > > Documentation/process/volatile-considered-harmful.rst > > Signed-off-by: MugilRaj > > Please put a blank before the Signed-off-by line. > > Probably there should be a space between your first and last name. It's > supposed to your legal name like for signing a legal document so use > whatever is appropriate legal documents in your country. > > Also the Documentation/process/volatile-considered-harmful.rst explains > that people often use "volatile" when they should be using locking for > synchronization. That seems to be the case here. So the correct fix is > to add locking. That's a little bit complicated to do and requires > testing. > > If we apply this patch, then we have silenced the warning so now someone > will have to look for the bug. But if we leave it as-is, then everyone > will know that the code is buggy. So let's leave it as-is until we are > able to fix the bug. > > It's always better to have easy to find bugs, than hidden bugs. And better still to comment known opportunities to improve the code so the next time someone tries to remove this volatile, there's a comment right there showing what's necessary instead. Something like: --- drivers/staging/speakup/speakup_decext.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/staging/speakup/speakup_decext.c b/drivers/staging/speakup/speakup_decext.c index 7408eb29cf38..e68e6046bb51 100644 --- a/drivers/staging/speakup/speakup_decext.c +++ b/drivers/staging/speakup/speakup_decext.c @@ -21,6 +21,7 @@ #define SYNTH_CLEAR 0x03 #define PROCSPEECH 0x0b +/* TODO: Remove volatile, maybe add locks to read_buff_add and synth_full() ? */ static volatile unsigned char last_char; static void read_buff_add(u_char ch) ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] taging: speakup: remove volatile
On Fri, 2020-05-22 at 19:13 +0200, Samuel Thibault wrote: > Joe Perches, le ven. 22 mai 2020 09:36:05 -0700, a ecrit: > > On Fri, 2020-05-22 at 13:34 +0300, Dan Carpenter wrote: > > > On Fri, May 22, 2020 at 02:46:28PM +0530, MugilRaj wrote: > > > > fix checkpatch.pl warning, which is Use of volatile is usually wrong: > > > > see > > > > Documentation/process/volatile-considered-harmful.rst > > > > Signed-off-by: MugilRaj > > > > > > Please put a blank before the Signed-off-by line. > > > > > > Probably there should be a space between your first and last name. It's > > > supposed to your legal name like for signing a legal document so use > > > whatever is appropriate legal documents in your country. > > > > > > Also the Documentation/process/volatile-considered-harmful.rst explains > > > that people often use "volatile" when they should be using locking for > > > synchronization. That seems to be the case here. So the correct fix is > > > to add locking. That's a little bit complicated to do and requires > > > testing. > > > > > > If we apply this patch, then we have silenced the warning so now someone > > > will have to look for the bug. But if we leave it as-is, then everyone > > > will know that the code is buggy. So let's leave it as-is until we are > > > able to fix the bug. > > > > > > It's always better to have easy to find bugs, than hidden bugs. > > > > And better still to comment known opportunities to > > improve the code so the next time someone tries to > > remove this volatile, there's a comment right there > > showing what's necessary instead. > > Actually I don't think adding the suggestion is a good thing if it's > only a "rule-of-thumb-replace-volatile-with-lock". > > Actually possibly volatile might not even be needed because there could > be already a lock protecting this. > > Put another way: I don't think putting any hint here would help, on the > contrary, somebody has to really look at what protection is needed, > without getting influenced by rules-of-thumb. checkpatch newbies/robots will submit this change again otherwise. Comment wording can always be improved. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC PATCH] staging: rtl8192u: indicate_packets() can be static
On Mon, 2020-05-18 at 04:22 +0800, kbuild test robot wrote:
> Signed-off-by: kbuild test robot
> ---
This doesn't apply on Linus' tree or -next so perhaps the
robot should put what tree and branch patches like these
are meant to be applied on after the --- line
> ieee80211_rx.c |2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c
> b/drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c
> index 3309f64be4c94..bceff1ba3d7d4 100644
> --- a/drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c
> +++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c
> @@ -520,7 +520,7 @@ static bool AddReorderEntry(struct rx_ts_record *pTS,
> struct rx_reorder_entry *p
> return true;
> }
>
> -void indicate_packets(struct ieee80211_device *ieee, struct ieee80211_rxb
> *rxb)
> +static void indicate_packets(struct ieee80211_device *ieee, struct
> ieee80211_rxb *rxb)
> {
> struct net_device_stats *stats = >stats;
> struct net_device *dev = ieee->dev;
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] staging: wilc1000: Increase the size of wid_list array
On Sun, 2020-05-03 at 14:52 +, ajay.kat...@microchip.com wrote:
> On 03/05/20 1:21 pm, Oscar Carter wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> > content is safe
> >
> > Increase by one the size of wid_list array as index variable can reach a
> > value of 5. If this happens, an out-of-bounds access is performed.
> >
> > Addresses-Coverity-ID: 1451981 ("Out-of-bounds access")
> > Fixes: f5a3cb90b802d ("staging: wilc1000: add passive scan support")
> > Signed-off-by: Oscar Carter
[]
> > diff --git a/drivers/staging/wilc1000/hif.c b/drivers/staging/wilc1000/hif.c
[]
> > @@ -151,7 +151,7 @@ int wilc_scan(struct wilc_vif *vif, u8 scan_source, u8
> > scan_type,
> > void *user_arg, struct cfg80211_scan_request *request)
> > {
> > int result = 0;
> > - struct wid wid_list[5];
> > + struct wid wid_list[6];
This looks like it should be using a #define instead of
a hard-coded number.
> > u32 index = 0;
> > u32 i, scan_timeout;
> > u8 *buffer;
> > --
> > 2.20.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/2] staging: vt6655: fix LONG_LINE warning
On Sun, 2020-05-03 at 00:16 +0200, Matej Dujava wrote:
> This patch will fix LONG_LINE error from checkpatch, by createing temporary
> variable so call to the function is not in if/else block.
[]
> diff --git a/drivers/staging/vt6655/rxtx.c b/drivers/staging/vt6655/rxtx.c
[]
> @@ -164,16 +164,24 @@ s_uGetTxRsvTime(
> )
> {
> unsigned int uDataTime, uAckTime;
> + unsigned short basic_rate;
>
> uDataTime = bb_get_frame_time(pDevice->byPreambleType, byPktType,
> cbFrameLength, wRate);
>
> if (!bNeedAck)
> return uDataTime;
>
> - if (byPktType == PK_TYPE_11B) /* llb,CCK mode */
> - uAckTime = bb_get_frame_time(pDevice->byPreambleType,
> byPktType, 14, (unsigned short)pDevice->byTopCCKBasicRate);
> - else /* 11g 2.4G OFDM mode & 11a 5G OFDM mode */
> - uAckTime = bb_get_frame_time(pDevice->byPreambleType,
> byPktType, 14, (unsigned short)pDevice->byTopOFDMBasicRate);
> + /*
> + * CCK mode - 11b
> + * OFDM mode - 11g 2.4G & 11a 5G
> + */
> + if (byPktType == PK_TYPE_11B)
> + basic_rate = (unsigned short)pDevice->byTopCCKBasicRate;
> + else
> + basic_rate = (unsigned short)pDevice->byTopOFDMBasicRate;
> +
> + uAckTime = bb_get_frame_time(pDevice->byPreambleType, byPktType, 14,
> + basic_rate);
>
> return uDataTime + pDevice->uSIFS + uAckTime;
> }
perhaps simpler using a ?:
uAckTime = bb_get_frame_time(pDevice->byPreambleType, byPktType, 14,
byPktType == PK_TYPE_11B
? pDevice->byTopCCKBasicRate
: pDevice->byTopOFDMBasicRate);
the casts aren't necessary either as both by... fields are u8
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 2/7] staging: qlge: Remove gotos from ql_set_mac_addr_reg
On Thu, 2020-04-30 at 12:38 +0300, Dan Carpenter wrote: > On Wed, Apr 29, 2020 at 09:33:04PM -0400, Rylan Dmello wrote: > > As suggested by Joe Perches, this patch removes the 'exit' label > > from the ql_set_mac_addr_reg function and replaces the goto > > statements with break statements. [] > > diff --git a/drivers/staging/qlge/qlge_main.c > > b/drivers/staging/qlge/qlge_main.c [] > > @@ -336,22 +336,20 @@ static int ql_set_mac_addr_reg(struct ql_adapter > > *qdev, u8 *addr, u32 type, > > > > status = ql_wait_reg_rdy(qdev, MAC_ADDR_IDX, MAC_ADDR_MW, 0); > > if (status) > > - goto exit; > > + break; > > Just "return status". A direct return is immediately clear but with a > break statement then you have to look down a bit and then scroll back. To me, 6 of 1, half dozen of other as all the case breaks could be returns. So either form is fine with me. The old form was poor through. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 0/7] staging: qlge: Checkpatch.pl indentation fixes in qlge_main.c
On Wed, 2020-04-29 at 21:31 -0400, Rylan Dmello wrote: > This patchset fixes some indentation- and style-related issues in qlge_main.c > reported by checkpatch.pl, such as: > > WARNING: Avoid multiple line dereference > WARNING: line over 80 characters > WARNING: suspect code indent for conditional statements All of this looks reasonable to me. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/3] staging: qlge: Remove multi-line dereferences from qlge_main.c
On Wed, 2020-04-29 at 00:04 -0400, Rylan Dmello wrote:
> Fix checkpatch.pl warnings:
>
> WARNING: Avoid multiple line dereference - prefer 'qdev->func'
> WARNING: Avoid multiple line dereference - prefer 'qdev->flags'
Assuming you are doing this for exercise:
It'd be better to unindent all the switch/case
blocks for the entire function so more functions
fit on single lines
switch (foo) {
case bar:
{
...;
should be:
switch (foo) {
case bar: {
...;
goto exit; might as well be break; and remove
the exit label too.
> Signed-off-by: Rylan Dmello
> ---
> drivers/staging/qlge/qlge_main.c | 9 -
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/staging/qlge/qlge_main.c
> b/drivers/staging/qlge/qlge_main.c
> index d7e4dfafc1a3..10daae025790 100644
> --- a/drivers/staging/qlge/qlge_main.c
> +++ b/drivers/staging/qlge/qlge_main.c
> @@ -396,8 +396,7 @@ static int ql_set_mac_addr_reg(struct ql_adapter *qdev,
> u8 *addr, u32 type,
>* the route field to NIC core.
>*/
> cam_output = (CAM_OUT_ROUTE_NIC |
> - (qdev->
> -func << CAM_OUT_FUNC_SHIFT) |
> + (qdev->func << CAM_OUT_FUNC_SHIFT) |
> (0 << CAM_OUT_CQ_ID_SHIFT));
> if (qdev->ndev->features & NETIF_F_HW_VLAN_CTAG_RX)
> cam_output |= CAM_OUT_RV;
> @@ -3432,9 +3431,9 @@ static int ql_request_irq(struct ql_adapter *qdev)
>>rx_ring[0]);
> status =
> request_irq(pdev->irq, qlge_isr,
> - test_bit(QL_MSI_ENABLED,
> - >
> - flags) ? 0 : IRQF_SHARED,
> + test_bit(QL_MSI_ENABLED, >flags)
> + ? 0
> + : IRQF_SHARED,
> intr_context->name, >rx_ring[0]);
> if (status)
> goto err_irq;
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/2] drivers: staging: rts5208: rtsx.c fix Unbalanced braces around else-if statements
On Mon, 2020-04-27 at 18:04 +0200, Greg KH wrote: > On Mon, Apr 27, 2020 at 04:24:42PM +0100, John Oldman wrote: > > Make a better job of fixing coding style issues, this time fixing > > all blocks as per Joe Perches' comment. [] > Why is this not just one patch? Hi John. These _should_ be one patch. The subject line should be versioned and affirmative like: Subject: [PATCH V2] staging: rts5208: rtsx: Use balanced braces for if/else if blocks The subject should not include "fix", as this is a coding style issue and not really a logical defect. Lastly the "this time fixing" bit should be below the --- line. cheers, Joe ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] drivers: staging: rts5208: rtsx.c fix Unbalanced braces around else statement issue
On Sun, 2020-04-26 at 17:02 +0100, John Oldman wrote:
> Fix coding style issue
>
> Signed-off-by: John Oldman
> ---
> drivers/staging/rts5208/rtsx.c | 6 ++
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/rts5208/rtsx.c b/drivers/staging/rts5208/rtsx.c
> index be0053c795b7..ca836ca2ee81 100644
> --- a/drivers/staging/rts5208/rtsx.c
> +++ b/drivers/staging/rts5208/rtsx.c
> @@ -419,10 +419,8 @@ static int rtsx_control_thread(void *__dev)
> chip->srb->device->id,
> (u8)chip->srb->device->lun);
> chip->srb->result = DID_BAD_TARGET << 16;
> - }
> -
> - /* we've got a command, let's do it! */
> - else {
> + } else {
> + /* we've got a command, let's do it! */
> scsi_show_command(chip);
> rtsx_invoke_transport(chip->srb, chip);
> }
Please look at the code blocks you are changing and
if you are trying to fix unbalanced else blocks, do
all the blocks, not just one of many consecutive.
/* reject the command if the direction indicator
* is UNKNOWN
*/
if (chip->srb->sc_data_direction == DMA_BIDIRECTIONAL) {
dev_err(>pci->dev, "UNKNOWN data direction\n");
chip->srb->result = DID_ERROR << 16;
}
/* reject if target != 0 or if LUN is higher than
* the maximum known LUN
*/
else if (chip->srb->device->id) {
dev_err(>pci->dev, "Bad target number (%d:%d)\n",
chip->srb->device->id,
(u8)chip->srb->device->lun);
chip->srb->result = DID_BAD_TARGET << 16;
}
else if (chip->srb->device->lun > chip->max_lun) {
dev_err(>pci->dev, "Bad LUN (%d:%d)\n",
chip->srb->device->id,
(u8)chip->srb->device->lun);
chip->srb->result = DID_BAD_TARGET << 16;
}
/* we've got a command, let's do it! */
else {
scsi_show_command(chip);
rtsx_invoke_transport(chip->srb, chip);
}
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/3] staging: vt6656: Remove the local variable "array"
On Sat, 2020-04-25 at 14:38 +0200, Oscar Carter wrote:
> Remove the local variable "array" and all the memcpy function calls
> because this copy operation from different arrays to this variable is
> unnecessary.
You might write here that vnt_control_out already does
a kmemdup copy of its const char *buffer argument and
this was made unnecessary by:
commit 12ecd24ef93277e4e5feaf27b0b18f2d3828bc5e
Author: Malcolm Priestley
Date: Sat Apr 22 11:14:57 2017 +0100
staging: vt6656: use off stack for out buffer USB transfers.
Since 4.9 mandated USB buffers be heap allocated this causes the driver
to fail.
Since there is a wide range of buffer sizes use kmemdup to create
allocated buffer.
> The same result can be achieved using the arrays directly.
>
> Signed-off-by: Oscar Carter
> ---
> drivers/staging/vt6656/rf.c | 21 +
> 1 file changed, 5 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/staging/vt6656/rf.c b/drivers/staging/vt6656/rf.c
> index 06fa8867cfa3..82d3b6081b5b 100644
> --- a/drivers/staging/vt6656/rf.c
> +++ b/drivers/staging/vt6656/rf.c
> @@ -770,7 +770,6 @@ int vnt_rf_table_download(struct vnt_private *priv)
> u16 length1 = 0, length2 = 0, length3 = 0;
> u8 *addr1 = NULL, *addr2 = NULL, *addr3 = NULL;
> u16 length, value;
> - u8 array[256];
>
> switch (priv->rf_type) {
> case RF_AL2230:
> @@ -817,10 +816,8 @@ int vnt_rf_table_download(struct vnt_private *priv)
> }
>
> /* Init Table */
> - memcpy(array, addr1, length1);
> -
> ret = vnt_control_out(priv, MESSAGE_TYPE_WRITE, 0,
> - MESSAGE_REQUEST_RF_INIT, length1, array);
> + MESSAGE_REQUEST_RF_INIT, length1, addr1);
> if (ret)
> goto end;
>
> @@ -832,10 +829,8 @@ int vnt_rf_table_download(struct vnt_private *priv)
> else
> length = length2;
>
> - memcpy(array, addr2, length);
> -
> ret = vnt_control_out(priv, MESSAGE_TYPE_WRITE, value,
> - MESSAGE_REQUEST_RF_CH0, length, array);
> + MESSAGE_REQUEST_RF_CH0, length, addr2);
> if (ret)
> goto end;
>
> @@ -852,10 +847,8 @@ int vnt_rf_table_download(struct vnt_private *priv)
> else
> length = length3;
>
> - memcpy(array, addr3, length);
> -
> ret = vnt_control_out(priv, MESSAGE_TYPE_WRITE, value,
> - MESSAGE_REQUEST_RF_CH1, length, array);
> + MESSAGE_REQUEST_RF_CH1, length, addr3);
> if (ret)
> goto end;
>
> @@ -870,11 +863,9 @@ int vnt_rf_table_download(struct vnt_private *priv)
> addr1 = _init_table_amode[0][0];
> addr2 = _channel_table2[0][0];
>
> - memcpy(array, addr1, length1);
> -
> /* Init Table 2 */
> ret = vnt_control_out(priv, MESSAGE_TYPE_WRITE, 0,
> - MESSAGE_REQUEST_RF_INIT2, length1, array);
> + MESSAGE_REQUEST_RF_INIT2, length1, addr1);
> if (ret)
> goto end;
>
> @@ -886,11 +877,9 @@ int vnt_rf_table_download(struct vnt_private *priv)
> else
> length = length2;
>
> - memcpy(array, addr2, length);
> -
> ret = vnt_control_out(priv, MESSAGE_TYPE_WRITE, value,
> MESSAGE_REQUEST_RF_CH2, length,
> - array);
> + addr2);
> if (ret)
> goto end;
>
> --
> 2.20.1
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 7/9] media: MAINTAINERS: rkisp1: add path to dt-bindings
On Fri, 2020-04-17 at 09:18 +0200, Hans Verkuil wrote: > On 03/04/2020 18:15, Helen Koike wrote: > > The Rockchip ISP bindings was moved out of staging. > > Update MAINTAINERS file with the new path. > > Shouldn't there be a reference to > Documentation/devicetree/bindings/phy/rockchip-mipi-dphy-rx0.yaml > as well in MAINTAINERS? And please keep the file references F: fields in alphabetic order. > > diff --git a/MAINTAINERS b/MAINTAINERS [] > > @@ -14303,6 +14303,7 @@ M: Helen Koike > > L: linux-me...@vger.kernel.org > > S: Maintained > > F: drivers/staging/media/rkisp1/ > > +F: Documentation/devicetree/bindings/media/rockchip-isp1.yaml > > > > ROCKCHIP RASTER 2D GRAPHIC ACCELERATION UNIT DRIVER > > M: Jacob Chen > > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/2] staging: rtl8192u: Refactoring setKey function
On Tue, 2020-04-14 at 22:18 -0300, Camylla Cantanheide wrote: > Em ter., 14 de abr. de 2020 às 09:35, Dan Carpenter < > dan.carpen...@oracle.com> escreveu: [] > > > diff --git a/drivers/staging/rtl8192u/r8192U_core.c [] > > > + target_content = macaddr[0] << 16 | > > > + macaddr[0] << 24 | > > > + (u32)us_config; And Camylla, this statement should be + target_content = macaddr[0] << 16 | + macaddr[1] << 24 | + (u32)us_config; Not a repeated [0] > > > + > > > + write_nic_dword(dev, WCAMI, target_content); > > > + write_nic_dword(dev, RWCAM, target_command++); > > > + > > > + /* MAC */ > > > + target_content = macaddr[2] | > > > + macaddr[3] << 8 | > > > + macaddr[4] << 16 | > > > + macaddr[5] << 24; ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 2/2] crypto: Remove unnecessary memzero_explicit()
On Tue, 2020-04-14 at 15:37 -0400, Waiman Long wrote: > OK, I can change it to clear the key length when the allocation failed > which isn't likely. Perhaps: kfree_sensitive(op->key); op->key = NULL; op->keylen = 0; but I don't know that it impacts any possible state. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
