Re: [PATCH 1/1] ARM: add printf format attribute to early_print()
On 28/08/16 18:58, Nicolas Iooss wrote:
> Adding such an attribute is helpful to detect errors related to printf
> formats at compile-time.
>
> Signed-off-by: Nicolas Iooss
> ---
> arch/arm/include/asm/setup.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm/include/asm/setup.h b/arch/arm/include/asm/setup.h
> index 3613d7e9fc40..797b8505b49a 100644
> --- a/arch/arm/include/asm/setup.h
> +++ b/arch/arm/include/asm/setup.h
> @@ -22,7 +22,7 @@
> static const struct tagtable __tagtable_##fn __tag = { tag, fn }
>
> extern int arm_add_memory(u64 start, u64 size);
> -extern void early_print(const char *str, ...);
> +extern __printf(1, 2) void early_print(const char *str, ...);
> extern void dump_machine_table(void);
>
> #ifdef CONFIG_ATAGS_PROC
Hello,
I sent this patch a few weeks ago and got no reply (and nothing showed
up on https://patchwork.kernel.org/patch/9302825/). Could you please
consider it for 4.9?
Thanks,
Nicolas
[PATCH 1/1] UBSAN: use uppercase K to format a kernel pointer
handle_object_size_mismatch() used %pk to format a kernel pointer in
pr_err(). This seems to be a misspelling for %pK.
Fixes: c6d308534aef ("UBSAN: run-time undefined behavior sanity checker")
Signed-off-by: Nicolas Iooss
---
lib/ubsan.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/ubsan.c b/lib/ubsan.c
index 8799ae5e2e42..d57d1e7e98a3 100644
--- a/lib/ubsan.c
+++ b/lib/ubsan.c
@@ -308,7 +308,7 @@ static void handle_object_size_mismatch(struct
type_mismatch_data *data,
return;
ubsan_prologue(&data->location, &flags);
- pr_err("%s address %pk with insufficient space\n",
+ pr_err("%s address %pK with insufficient space\n",
type_check_kinds[data->type_check_kind],
(void *) ptr);
pr_err("for an object of type %s\n", data->type->type_name);
--
2.9.0
[PATCH 1/1] Bluetooth: add printf format attribute to hci_set_[fh]w_info()
Commit 5177a83827cd ("Bluetooth: Add debugfs fields for hardware and
firmware info") introduced hci_set_hw_info() and hci_set_fw_info().
These functions use kvasprintf_const() but are not marked with a
__printf attribute. Adding such an attribute helps detecting issues
related to printf-formatting at build time.
Signed-off-by: Nicolas Iooss
---
include/net/bluetooth/hci_core.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index ee7fc47680a1..012e5031fe47 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1026,8 +1026,8 @@ int hci_resume_dev(struct hci_dev *hdev);
int hci_reset_dev(struct hci_dev *hdev);
int hci_recv_frame(struct hci_dev *hdev, struct sk_buff *skb);
int hci_recv_diag(struct hci_dev *hdev, struct sk_buff *skb);
-void hci_set_hw_info(struct hci_dev *hdev, const char *fmt, ...);
-void hci_set_fw_info(struct hci_dev *hdev, const char *fmt, ...);
+__printf(2, 3) void hci_set_hw_info(struct hci_dev *hdev, const char *fmt,
...);
+__printf(2, 3) void hci_set_fw_info(struct hci_dev *hdev, const char *fmt,
...);
int hci_dev_open(__u16 dev);
int hci_dev_close(__u16 dev);
int hci_dev_do_close(struct hci_dev *hdev);
--
2.9.0
[PATCH 1/1] x86/entry: spell EBX register correctly in documentation
As EBS does not mean anything reasonable in the context it is used, it seems like a misspelling for EBX. Signed-off-by: Nicolas Iooss --- arch/x86/entry/entry_64.S | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S index b846875aeea6..6f3076302017 100644 --- a/arch/x86/entry/entry_64.S +++ b/arch/x86/entry/entry_64.S @@ -1058,7 +1058,7 @@ END(error_entry) /* - * On entry, EBS is a "return to kernel mode" flag: + * On entry, EBX is a "return to kernel mode" flag: * 1: already in kernel mode, don't need SWAPGS * 0: user gsbase is loaded, we need SWAPGS and standard preparation for return to usermode */ -- 2.9.0
Re: [PATCH 1/1] UBSAN: use uppercase K to format a kernel pointer
On 07/29/2016 09:53 PM, Joe Perches wrote:
> On Fri, 2016-07-29 at 13:10 +0200, Nicolas Iooss wrote:
>> handle_object_size_mismatch() used %pk to format a kernel pointer in
>> pr_err(). This seems to be a misspelling for %pK.
>
> Thanks
Thanks for your feedbacks. I agree %pK does not make much sense in the
context it is used. I will modify this patch to use %p instead.
>> diff --git a/lib/ubsan.c b/lib/ubsan.c
> []
>> @@ -308,7 +308,7 @@ static void handle_object_size_mismatch(struct
>> type_mismatch_data *data,
>> return;
>>
>> ubsan_prologue(&data->location, &flags);
>> -pr_err("%s address %pk with insufficient space\n",
>> +pr_err("%s address %pK with insufficient space\n",
>> type_check_kinds[data->type_check_kind],
>> (void *) ptr);
>> pr_err("for an object of type %s\n", data->type->type_name);
>
> Maybe change this to a single output line:
>
> pr_err("%s address %pK with insufficient space for an object of type
> %s\n",
> type_check_kinds[data->type_check_kind], (void *)ptr,
> data->type->type_name);
As both handle_missaligned_access() and handle_object_size_mismatch()
use two pr_err() calls to display their error messages, it seems the
split has been made on purpose (maybe to avoid logging long lines).
I won't merge the calls in my patch as this appears to be more an
ergonomic subject for people really using this code.
-- Nicolas
[PATCH 1/1] UBSAN: fix typo in format string
handle_object_size_mismatch() used %pk to format a kernel pointer with
pr_err(). This seemed to be a misspelling for %pK, but using this to
format a kernel pointer does not make much sence here.
Therefore use %p instead, like in handle_missaligned_access().
Signed-off-by: Nicolas Iooss
---
lib/ubsan.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/ubsan.c b/lib/ubsan.c
index 8799ae5e2e42..fb0409df1bcf 100644
--- a/lib/ubsan.c
+++ b/lib/ubsan.c
@@ -308,7 +308,7 @@ static void handle_object_size_mismatch(struct
type_mismatch_data *data,
return;
ubsan_prologue(&data->location, &flags);
- pr_err("%s address %pk with insufficient space\n",
+ pr_err("%s address %p with insufficient space\n",
type_check_kinds[data->type_check_kind],
(void *) ptr);
pr_err("for an object of type %s\n", data->type->type_name);
--
2.9.0
Re: [Intel-gfx] [PATCH 1/1] drm/i915/dsi: silence a warning about uninitialized return value
On 06/09/16 12:21, Dave Gordon wrote:
> On 04/09/16 19:58, Nicolas Iooss wrote:
>> When building the kernel with clang and some warning flags, the compiler
>> reports that the return value of dcs_get_backlight() may be
>> uninitialized:
>>
>> drivers/gpu/drm/i915/intel_dsi_dcs_backlight.c:53:2: error: variable
>> 'data' is used uninitialized whenever 'for' loop exits because its
>> condition is false [-Werror,-Wsometimes-uninitialized]
>> for_each_dsi_port(port, intel_dsi->dcs_backlight_ports) {
>> ^~~
>> drivers/gpu/drm/i915/intel_dsi.h:126:49: note: expanded from macro
>> 'for_each_dsi_port'
>> #define for_each_dsi_port(__port, __ports_mask)
>> for_each_port_masked(__port,
>> __ports_mask)
>>
>> ^~
>> drivers/gpu/drm/i915/i915_drv.h:322:26: note: expanded from macro
>> 'for_each_port_masked'
>> for ((__port) = PORT_A; (__port) < I915_MAX_PORTS; (__port)++) \
>> ^
>> drivers/gpu/drm/i915/intel_dsi_dcs_backlight.c:60:9: note:
>> uninitialized use occurs here
>> return data;
>>^~~~
>>
>> As intel_dsi->dcs_backlight_ports seems to be always initialized to a
>> non-null value, the content of the for loop is always executed and there
>> is no bug in the current code. Nevertheless the compiler has no way of
>> knowing that assumption, so initialize variable 'data' to silence the
>> warning here.
>>
>> Signed-off-by: Nicolas Iooss
>
> Interesting ... there are two things that could lead to this (possibly)
> incorrect analysis. Either it thinks the loop could be executed zero
> times, which would be a deficiency in the compiler, as the initialiser
> and loop bound are both known (different) constants:
>
> enum port {
> PORT_A = 0,
> PORT_B,
> PORT_C,
> PORT_D,
> PORT_E,
> I915_MAX_PORTS
> };
>
> or, it doesn't understand that because we've passed &data to another
> function, it can have been set by the callee. It may be extra confusing
> that the callee takes (void *); or it may be being ultra-sophisticated
> in its analysis and noted that in one error path data is *not* set (and
> we then discard the error and use data anyway). As an experiment, you
> could try:
The code that the compiler sees is not a simple loop other enum 'port'
but "for_each_dsi_port(port, intel_dsi->dcs_backlight_ports) {", which
is expanded [1] to:
for ((port) = PORT_A; (port) < I915_MAX_PORTS; (port)++)
if (!((intel_dsi->dcs_backlight_ports) & (1 << (port {} else {
This is why I spoke of intel_dsi->dcs_backlight_ports in my description:
if it is zero, the body of the loop is never run.
As for the analyses of calls using &data, clang does not warn about the
variable being maybe uninitialized following a call. This is quite
expected as this would lead to too many false positives, even though it
may miss some bugs.
>
> static u8 mipi_dsi_dcs_read1(struct mipi_dsi_device *dsi_device, u8 cmd)
> {
> u8 data = 0;
>
> mipi_dsi_dcs_read(dsi_device, cmd, &data, sizeof(data));
>
> return data;
> }
>
> static u32 dcs_get_backlight(struct intel_connector *connector)
> {
> struct intel_encoder *encoder = connector->encoder;
> struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
> struct mipi_dsi_device *dsi_device;
> enum port port;
> u8 data;
>
> /* FIXME: Need to take care of 16 bit brightness level */
> for_each_dsi_port(port, intel_dsi->dcs_backlight_ports) {
> dsi_device = intel_dsi->dsi_hosts[port]->device;
> data = mipi_dsi_dcs_read1(dsi_device,
> MIPI_DCS_GET_DISPLAY_BRIGHTNESS);
> break;
> }
>
> return data;
> }
>
> If it complains about that then it's a shortcoming in the loop analysis.
It complains (in dcs_get_backlight), because for_each_dsi_port() still
hides an 'if' condition.
> If not you could try:
>
> static u8 mipi_dsi_dcs_read1(struct mipi_dsi_device *dsi_device, u8 cmd)
> {
> u8 data;
> ssize_t nbytes = sizeof(data);
>
> nbytes = mipi_dsi_dcs_read(dsi_device, cmd, &data, nbytes);
> return nbytes == sizeof(data) ? data : 0;
> }
>
> and if complains about that then it doesn't understand that passing
> &data allows it to be set. If it doesn't complain about this version,
> then the original error was actually correct, in the sense that data can
> indeed be used uninitialised if certain error paths can be taken.
clang did not complain with this last case.
> Here's an R-b for your fix anyway ...
>
> Reviewed-by: Dave Gordon
Thanks!
Nicolas
[1] I used "make drivers/gpu/drm/i915/intel_dsi_dcs_backlight.i" to see
the output of the preprocessor.
Re: [Intel-gfx] [PATCH 1/1] drm/i915/dsi: silence a warning about uninitialized return value
On 07/09/16 18:03, Dave Gordon wrote:
> On 06/09/16 21:36, Nicolas Iooss wrote:
>> On 06/09/16 12:21, Dave Gordon wrote:
>>> On 04/09/16 19:58, Nicolas Iooss wrote:
>>>> When building the kernel with clang and some warning flags, the
>>>> compiler
>>>> reports that the return value of dcs_get_backlight() may be
>>>> uninitialized:
>>>>
>>>> drivers/gpu/drm/i915/intel_dsi_dcs_backlight.c:53:2: error:
>>>> variable
>>>> 'data' is used uninitialized whenever 'for' loop exits because its
>>>> condition is false [-Werror,-Wsometimes-uninitialized]
>>>> for_each_dsi_port(port, intel_dsi->dcs_backlight_ports) {
>>>> ^~~
>>>> drivers/gpu/drm/i915/intel_dsi.h:126:49: note: expanded from macro
>>>> 'for_each_dsi_port'
>>>> #define for_each_dsi_port(__port, __ports_mask)
>>>> for_each_port_masked(__port,
>>>> __ports_mask)
>>>>
>>>> ^~
>>>> drivers/gpu/drm/i915/i915_drv.h:322:26: note: expanded from macro
>>>> 'for_each_port_masked'
>>>> for ((__port) = PORT_A; (__port) < I915_MAX_PORTS;
>>>> (__port)++) \
>>>> ^
>>>> drivers/gpu/drm/i915/intel_dsi_dcs_backlight.c:60:9: note:
>>>> uninitialized use occurs here
>>>> return data;
>>>> ^~~~
>>>>
>>>> As intel_dsi->dcs_backlight_ports seems to be always initialized to a
>>>> non-null value, the content of the for loop is always executed and
>>>> there
>>>> is no bug in the current code. Nevertheless the compiler has no way of
>>>> knowing that assumption, so initialize variable 'data' to silence the
>>>> warning here.
>>>>
>>>> Signed-off-by: Nicolas Iooss
>>>
>>> Interesting ... there are two things that could lead to this (possibly)
>>> incorrect analysis. Either it thinks the loop could be executed zero
>>> times, which would be a deficiency in the compiler, as the initialiser
>>> and loop bound are both known (different) constants:
>>>
>>> enum port {
>>> PORT_A = 0,
>>> PORT_B,
>>> PORT_C,
>>> PORT_D,
>>> PORT_E,
>>> I915_MAX_PORTS
>>> };
>>>
>>> or, it doesn't understand that because we've passed &data to another
>>> function, it can have been set by the callee. It may be extra confusing
>>> that the callee takes (void *); or it may be being ultra-sophisticated
>>> in its analysis and noted that in one error path data is *not* set (and
>>> we then discard the error and use data anyway). As an experiment, you
>>> could try:
>>
>> The code that the compiler sees is not a simple loop other enum 'port'
>> but "for_each_dsi_port(port, intel_dsi->dcs_backlight_ports) {", which
>> is expanded [1] to:
>>
>> for ((port) = PORT_A; (port) < I915_MAX_PORTS; (port)++)
>> if (!((intel_dsi->dcs_backlight_ports) & (1 << (port {} else {
>>
>> This is why I spoke of intel_dsi->dcs_backlight_ports in my description:
>> if it is zero, the body of the loop is never run.
>>
>> As for the analyses of calls using &data, clang does not warn about the
>> variable being maybe uninitialized following a call. This is quite
>> expected as this would lead to too many false positives, even though it
>> may miss some bugs.
>>
>>> static u8 mipi_dsi_dcs_read1(struct mipi_dsi_device *dsi_device, u8 cmd)
>>> {
>>> u8 data = 0;
>>>
>>> mipi_dsi_dcs_read(dsi_device, cmd, &data, sizeof(data));
>>>
>>> return data;
>>> }
>>>
>>> static u32 dcs_get_backlight(struct intel_connector *connector)
>>> {
>>> struct intel_encoder *encoder = connector->encoder;
>>> struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
>>> struct mipi_dsi_device *dsi_device;
>>> enum port port;
>>> u8 data;
>>>
>>> /* FIXME: Need to take care of 16 bit brightness level */
>>> for
[PATCH 1/1] RDS: add __printf format attribute to error reporting functions
This is helpful to detect at compile-time errors related to format strings. Signed-off-by: Nicolas Iooss --- net/rds/ib.h | 1 + net/rds/rds.h | 1 + 2 files changed, 2 insertions(+) diff --git a/net/rds/ib.h b/net/rds/ib.h index 046f7508c06b..45ac8e8e58f4 100644 --- a/net/rds/ib.h +++ b/net/rds/ib.h @@ -333,6 +333,7 @@ void rds_ib_conn_path_shutdown(struct rds_conn_path *cp); void rds_ib_state_change(struct sock *sk); int rds_ib_listen_init(void); void rds_ib_listen_stop(void); +__printf(2, 3) void __rds_ib_conn_error(struct rds_connection *conn, const char *, ...); int rds_ib_cm_handle_connect(struct rdma_cm_id *cm_id, struct rdma_cm_event *event); diff --git a/net/rds/rds.h b/net/rds/rds.h index b2d17f0fafa8..fd0bccb2f9f9 100644 --- a/net/rds/rds.h +++ b/net/rds/rds.h @@ -688,6 +688,7 @@ void __rds_conn_error(struct rds_connection *conn, const char *, ...); #define rds_conn_error(conn, fmt...) \ __rds_conn_error(conn, KERN_WARNING "RDS: " fmt) +__printf(2, 3) void __rds_conn_path_error(struct rds_conn_path *cp, const char *, ...); #define rds_conn_path_error(cp, fmt...) \ __rds_conn_path_error(cp, KERN_WARNING "RDS: " fmt) -- 2.9.2
[PATCH 1/1] brcmfmac: fix pmksa->bssid usage
The struct cfg80211_pmksa defines its bssid field as:
const u8 *bssid;
contrary to struct brcmf_pmksa, which uses:
u8 bssid[ETH_ALEN];
Therefore in brcmf_cfg80211_del_pmksa(), &pmksa->bssid takes the address
of this field (of type u8**), not the one of its content (which would be
u8*). Remove the & operator to make brcmf_dbg("%pM") and memcmp()
behave as expected.
This bug have been found using a custom static checker (which checks the
usage of %p... attributes at build time). It has been introduced in
commit 6c404f34f2bd ("brcmfmac: Cleanup pmksa cache handling code"),
which replaced pmksa->bssid by &pmksa->bssid while refactoring the code,
without modifying struct cfg80211_pmksa definition.
Fixes: 6c404f34f2bd ("brcmfmac: Cleanup pmksa cache handling code")
Cc: sta...@ger.kernel.org
Signed-off-by: Nicolas Iooss
---
scripts/checkpatch.pl reports a warning: "Prefer ether_addr_equal() or
ether_addr_equal_unaligned() over memcmp()". Because some files in
drivers/net/wireless/broadcom/brcm80211/brcmfmac/ still use memcmp()
to compare addresses and because I do not know whether pmksa->bssid is
always aligned, I did not follow this warning.
drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index 2628d5e12c64..aceab77cd95a 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -3884,11 +3884,11 @@ brcmf_cfg80211_del_pmksa(struct wiphy *wiphy, struct
net_device *ndev,
if (!check_vif_up(ifp->vif))
return -EIO;
- brcmf_dbg(CONN, "del_pmksa - PMK bssid = %pM\n", &pmksa->bssid);
+ brcmf_dbg(CONN, "del_pmksa - PMK bssid = %pM\n", pmksa->bssid);
npmk = le32_to_cpu(cfg->pmk_list.npmk);
for (i = 0; i < npmk; i++)
- if (!memcmp(&pmksa->bssid, &pmk[i].bssid, ETH_ALEN))
+ if (!memcmp(pmksa->bssid, &pmk[i].bssid, ETH_ALEN))
break;
if ((npmk > 0) && (i < npmk)) {
--
2.9.2
[PATCH 1/1] x86/mm: kaslr: fix -Wformat-security warning
debug_putstr() is used to output strings without using printf-like
formatting but debug_putstr(v) is defined as early_printk(v) in
arch/x86/lib/kaslr.c. This makes clang reports the following warning
when building with -Wformat-security:
arch/x86/lib/kaslr.c:57:15: warning: format string is not a string
literal (potentially insecure) [-Wformat-security]
debug_putstr(purpose);
^~~
Fix this by using "%s" in early_printk().
Signed-off-by: Nicolas Iooss
---
arch/x86/lib/kaslr.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/lib/kaslr.c b/arch/x86/lib/kaslr.c
index f7dfeda83e5c..121f59c6ee54 100644
--- a/arch/x86/lib/kaslr.c
+++ b/arch/x86/lib/kaslr.c
@@ -19,7 +19,7 @@
#include
#include
-#define debug_putstr(v) early_printk(v)
+#define debug_putstr(v) early_printk("%s", v)
#define has_cpuflag(f) boot_cpu_has(f)
#define get_boot_seed() kaslr_offset()
#endif
--
2.9.2
[PATCH 1/1] ASoc: simple-card-utils: add __printf attribute
asoc_simple_card_set_dailink_name() uses devm_kvasprintf() to format some of its arguments. Adding a __printf attribute to this function makes it possible to detect at compile-time errors related to format strings. Signed-off-by: Nicolas Iooss --- include/sound/simple_card_utils.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/sound/simple_card_utils.h b/include/sound/simple_card_utils.h index 86088aed9002..3207b1a70d38 100644 --- a/include/sound/simple_card_utils.h +++ b/include/sound/simple_card_utils.h @@ -27,6 +27,7 @@ int asoc_simple_card_parse_daifmt(struct device *dev, struct device_node *codec, char *prefix, unsigned int *retfmt); +__printf(3, 4) int asoc_simple_card_set_dailink_name(struct device *dev, struct snd_soc_dai_link *dai_link, const char *fmt, ...); -- 2.9.2
[PATCH v2 1/2] ASoC: simple-card-utils: add __printf attribute
asoc_simple_card_set_dailink_name() uses devm_kvasprintf() to format some of its arguments. Adding a __printf attribute to this function makes it possible to detect at compile-time errors related to format strings. Signed-off-by: Nicolas Iooss --- include/sound/simple_card_utils.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/sound/simple_card_utils.h b/include/sound/simple_card_utils.h index 86088aed9002..3207b1a70d38 100644 --- a/include/sound/simple_card_utils.h +++ b/include/sound/simple_card_utils.h @@ -27,6 +27,7 @@ int asoc_simple_card_parse_daifmt(struct device *dev, struct device_node *codec, char *prefix, unsigned int *retfmt); +__printf(3, 4) int asoc_simple_card_set_dailink_name(struct device *dev, struct snd_soc_dai_link *dai_link, const char *fmt, ...); -- 2.9.3
[PATCH v2 2/2] MAINTAINERS: document ASoC header files
include/sound/simple_card_utils.h is handled by ASoC maintainers, as stated in https://lkml.org/lkml/2016/8/22/307, and include/sound/simple_card.h seems to be an ASoC file too. In the future there will be more files named like these ones so introduce a pattern to match them. Signed-off-by: Nicolas Iooss --- MAINTAINERS | 1 + 1 file changed, 1 insertion(+) diff --git a/MAINTAINERS b/MAINTAINERS index 0bbe4b105c34..3f4e8bf042b9 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -11012,6 +11012,7 @@ S: Supported F: Documentation/devicetree/bindings/sound/ F: Documentation/sound/alsa/soc/ F: sound/soc/ +F: include/sound/simple*card* F: include/sound/soc* SOUND - DMAENGINE HELPERS -- 2.9.3
Re: [PATCH 1/1] drm/amd/powerplay: initialize a variable before using it
On Sun, Sep 3, 2017 at 2:00 PM, Nicolas Iooss
wrote:
>
> Function vega10_apply_state_adjust_rules() only initializes
> stable_pstate_sclk_dpm_percentage when
> data->registry_data.stable_pstate_sclk_dpm_percentage is not between 1
> and 100. The variable is then used to compute stable_pstate_sclk, which
> therefore uses an uninitialized value.
>
> Fix this by initializing stable_pstate_sclk_dpm_percentage to
> data->registry_data.stable_pstate_sclk_dpm_percentage.
>
> This issue has been found while building the kernel with clang. The
> compiler reported a -Wsometimes-uninitialized warning.
>
> Fixes: f83a9991648b ("drm/amd/powerplay: add Vega10 powerplay support (v5)")
> Signed-off-by: Nicolas Iooss
> ---
> drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
> b/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
> index 197174e562d2..c8d28f78cd47 100644
> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
> @@ -3043,6 +3043,8 @@ static int vega10_apply_state_adjust_rules(struct
> pp_hwmgr *hwmgr,
>
> if (phm_cap_enabled(hwmgr->platform_descriptor.platformCaps,
> PHM_PlatformCaps_StablePState)) {
> + stable_pstate_sclk_dpm_percentage =
> + data->registry_data.stable_pstate_sclk_dpm_percentage;
> PP_ASSERT_WITH_CODE(
> data->registry_data.stable_pstate_sclk_dpm_percentage
> >= 1 &&
> data->registry_data.stable_pstate_sclk_dpm_percentage
> <= 100,
> --
> 2.14.1
Hello,
I have not received any comment on the above patch that I sent two
months ago, and the issue which is fixed by it still exists in today's
linux-next code [1]. Could you please review this patch?
Thanks,
Nicolas
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c#n3137
[PATCH] input: elan_i2c - fix typo in include header guard
Signed-off-by: Nicolas Iooss
Fixes: 6696777c6506 ("Input: add driver for Elan I2C/SMbus touchpad")
---
drivers/input/mouse/elan_i2c.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/input/mouse/elan_i2c.h b/drivers/input/mouse/elan_i2c.h
index e100c1b31597..9b2dc015f20c 100644
--- a/drivers/input/mouse/elan_i2c.h
+++ b/drivers/input/mouse/elan_i2c.h
@@ -17,7 +17,7 @@
*/
#ifndef _ELAN_I2C_H
-#define _ELAN_i2C_H
+#define _ELAN_I2C_H
#include
--
2.3.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] mac802154: fix typo in header guard
Signed-off-by: Nicolas Iooss
Fixes: b6eea9ca354a ("mac802154: introduce driver-ops header")
---
net/mac802154/driver-ops.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/mac802154/driver-ops.h b/net/mac802154/driver-ops.h
index 98180a9fff4a..a0533357b9ea 100644
--- a/net/mac802154/driver-ops.h
+++ b/net/mac802154/driver-ops.h
@@ -1,4 +1,4 @@
-#ifndef __MAC802154_DRVIER_OPS
+#ifndef __MAC802154_DRIVER_OPS
#define __MAC802154_DRIVER_OPS
#include
@@ -220,4 +220,4 @@ drv_set_promiscuous_mode(struct ieee802154_local *local,
bool on)
return local->ops->set_promiscuous_mode(&local->hw, on);
}
-#endif /* __MAC802154_DRVIER_OPS */
+#endif /* __MAC802154_DRIVER_OPS */
--
2.3.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mac802154: fix typo in header guard
On 03/19/2015 09:46 PM, Paul Bolle wrote:
> On Thu, 2015-03-19 at 14:37 +0100, Alexander Aring wrote:
>> On Thu, Mar 19, 2015 at 09:23:40PM +0800, Nicolas Iooss wrote:
>>> Signed-off-by: Nicolas Iooss
>>> Fixes: b6eea9ca354a ("mac802154: introduce driver-ops header")
>>
>> Acked-by: Alexander Aring
>>
>> can you please queue this into bluetooth-next or even bluetooth?
>
> Is the Fixes: tag needed?
>
> mac802154.ko builds fine on my machine. There's also no error or warning
> included in the commit explanation. So it seems this is just a typo fix,
> not something that should be sent to stable too. Or did I miss something
> non-obvious?
No, you didn't miss anything. It is only a fix for a typo I found while
testing LLVMLinux (clang warns about such typos with -Wheader-guard).
I added a Fixes: tag because it is easier to remove it afterwards that
to search for a commit when adding it. I think this patch should not be
sent to stable@, and feel free to remove the tag if for you it means
"forward to stable@" (Documentation/SubmittingPatches is not clear about
whether a Fixes tag should be only used for real bugs or if it also
applies to compiler warnings).
Thanks for your quick replies.
Nicolas
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH RESENT] coredump: fix cn_printf formatting warnings
Add __printf attributes to cn_*printf functions. With these, gcc says:
fs/coredump.c:213:5: warning: format '%d' expects argument of type
'int', but argument 3 has type 'kuid_t' [-Wformat=]
err = cn_printf(cn, "%d", cred->uid);
^
fs/coredump.c:217:5: warning: format '%d' expects argument of type
'int', but argument 3 has type 'kgid_t' [-Wformat=]
err = cn_printf(cn, "%d", cred->gid);
^
fs/coredump.c:225:5: warning: format '%ld' expects argument of type
'long int', but argument 3 has type 'int' [-Wformat=]
err = cn_printf(cn, "%ld", cprm->siginfo->si_signo);
^
The third warning is easily fixed as si_signo is always an int.
For the two others, cred->uid and cred->gid need to be converted to
either a user-namespace UID/GID or to init_user_ns UID/GID. As
Documentation/sysctl/kernel.txt does not specify which user namespace is
used to translate %u and %g in core_pattern, but lowercase %p and %i are
used to format pid/tid in current process namespace, it seems intuitive
that lowercase %u and %g use the current user namespace. So implement
this.
Signed-off-by: Nicolas Iooss
---
I sent this patch more than a month ago but go no feedback, so I'm sending it
again.
Comments would be greatly appreciated.
fs/coredump.c | 18 --
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/fs/coredump.c b/fs/coredump.c
index bbbe139ab280..977fc8b91f2d 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -70,7 +70,8 @@ static int expand_corename(struct core_name *cn, int size)
return 0;
}
-static int cn_vprintf(struct core_name *cn, const char *fmt, va_list arg)
+static __printf(2, 0) int cn_vprintf(struct core_name *cn, const char *fmt,
+va_list arg)
{
int free, need;
va_list arg_copy;
@@ -93,7 +94,7 @@ again:
return -ENOMEM;
}
-static int cn_printf(struct core_name *cn, const char *fmt, ...)
+static __printf(2, 3) int cn_printf(struct core_name *cn, const char *fmt, ...)
{
va_list arg;
int ret;
@@ -105,7 +106,8 @@ static int cn_printf(struct core_name *cn, const char *fmt,
...)
return ret;
}
-static int cn_esc_printf(struct core_name *cn, const char *fmt, ...)
+static __printf(2, 3)
+int cn_esc_printf(struct core_name *cn, const char *fmt, ...)
{
int cur = cn->used;
va_list arg;
@@ -209,11 +211,15 @@ static int format_corename(struct core_name *cn, struct
coredump_params *cprm)
break;
/* uid */
case 'u':
- err = cn_printf(cn, "%d", cred->uid);
+ err = cn_printf(cn, "%d",
+ from_kuid_munged(cred->user_ns,
+cred->uid));
break;
/* gid */
case 'g':
- err = cn_printf(cn, "%d", cred->gid);
+ err = cn_printf(cn, "%d",
+ from_kgid_munged(cred->user_ns,
+cred->gid));
break;
case 'd':
err = cn_printf(cn, "%d",
@@ -221,7 +227,7 @@ static int format_corename(struct core_name *cn, struct
coredump_params *cprm)
break;
/* signal that caused the coredump */
case 's':
- err = cn_printf(cn, "%ld",
cprm->siginfo->si_signo);
+ err = cn_printf(cn, "%d",
cprm->siginfo->si_signo);
break;
/* UNIX time of coredump */
case 't': {
--
2.3.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] Revert "nfs: replace nfs_add_stats with nfs_inc_stats when add one"
This reverts commit 5a254d08b086d80cbead2ebcee6d2a4b3a15587a.
Since commit 5a254d08b086 ("nfs: replace nfs_add_stats with
nfs_inc_stats when add one"), nfs_readpage and nfs_do_writepage use
nfs_inc_stats to increment NFSIOS_READPAGES and NFSIOS_WRITEPAGES
instead of nfs_add_stats.
However nfs_inc_stats does not do the same thing as nfs_add_stats with
value 1 because these functions work on distinct stats:
nfs_inc_stats increments stats from "enum nfs_stat_eventcounters" (in
server->io_stats->events) and nfs_add_stats those from "enum
nfs_stat_bytecounters" (in server->io_stats->bytes).
Signed-off-by: Nicolas Iooss
---
As I haven't got any reply from the message I sent a few weeks ago, I
am now sending a patch.
More details about the reason of this revert can be found in my
previous e-mail, https://lkml.org/lkml/2015/3/24/226.
fs/nfs/read.c | 2 +-
fs/nfs/write.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/nfs/read.c b/fs/nfs/read.c
index 568ecf0a880f..848d8b1db4ce 100644
--- a/fs/nfs/read.c
+++ b/fs/nfs/read.c
@@ -284,7 +284,7 @@ int nfs_readpage(struct file *file, struct page *page)
dprintk("NFS: nfs_readpage (%p %ld@%lu)\n",
page, PAGE_CACHE_SIZE, page_file_index(page));
nfs_inc_stats(inode, NFSIOS_VFSREADPAGE);
- nfs_inc_stats(inode, NFSIOS_READPAGES);
+ nfs_add_stats(inode, NFSIOS_READPAGES, 1);
/*
* Try to flush any pending writes to the file..
diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index 759931088094..fae7f97ad34d 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -580,7 +580,7 @@ static int nfs_do_writepage(struct page *page, struct
writeback_control *wbc, st
int ret;
nfs_inc_stats(inode, NFSIOS_VFSWRITEPAGE);
- nfs_inc_stats(inode, NFSIOS_WRITEPAGES);
+ nfs_add_stats(inode, NFSIOS_WRITEPAGES, 1);
nfs_pageio_cond_complete(pgio, page_file_index(page));
ret = nfs_page_async_flush(pgio, page, wbc->sync_mode == WB_SYNC_NONE);
--
2.3.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] Staging: fbtft: fix header guard typo
drivers/staging/fbtft/internal.h header guard tests for __LINUX_FBTFT__INTERNAL_H but then defines __LINUX_FBTFT_INTERNAL_H (only 1 underscore) and uses the same name for the #endif comment. Use the same name everywhere. Signed-off-by: Nicolas Iooss --- drivers/staging/fbtft/internal.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/fbtft/internal.h b/drivers/staging/fbtft/internal.h index f69db8289151..eea0ec5ff4d3 100644 --- a/drivers/staging/fbtft/internal.h +++ b/drivers/staging/fbtft/internal.h @@ -13,7 +13,7 @@ * */ -#ifndef __LINUX_FBTFT__INTERNAL_H +#ifndef __LINUX_FBTFT_INTERNAL_H #define __LINUX_FBTFT_INTERNAL_H void fbtft_sysfs_init(struct fbtft_par *par); -- 2.3.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/1] security: Add CONFIG_LSM_AUTO to handle default LSM stack ordering
On Mon, Feb 22, 2021 at 9:32 PM Casey Schaufler wrote: > > On 2/22/2021 10:31 AM, Mickaël Salaün wrote: > > On 22/02/2021 17:51, Casey Schaufler wrote: > >> On 2/22/2021 7:06 AM, Mickaël Salaün wrote: > >>> From: Mickaël Salaün > >>> > >>> Add a new option CONFIG_LSM_AUTO to enable users to delegate default LSM > >>> stacking order to kernel developers. This enable to keep a consistent > >>> order of enabled LSM when changing the LSM selection, especially when a > >>> new LSM is added to the kernel. > >> TL;DR - NAK > >> > >> Do you think that we might have considered this when stacking was > >> introduced? > > I didn't dig the detailed history of LSM stacking, but you are in Cc > > because I know that you know. I may have though that the main goal of > > the current LSM stacking implementation was to enable to stack existing > > LSMs, which works well with this CONFIG_LSM list, but doesn't work as > > well for new LSMs. > > It works just fine for new LSMs if you treat them as significant > features which may have significant impact on the behavior of the > system. > > >> Did you even consider the implications before sending > >> the patch? > > Yes, and it doesn't change much the current behavior without user > > interaction. However, it gives the choice to users to choose how they > > want their configuration to evolve. > > Automatic inclusions of new LSMs would be counter to existing practice. > It won't work for "major" LSMs. > > > >> This only makes any sense if you want to compile in > >> AppArmor and/or Smack but always use SELinux. The existing Kconfig > >> model handles that perfectly well. > > This patch series doesn't change this behavior if the user doesn't want > > it to change. > > Well, there's the question. If a distribution/system uses the new scheme > "users" are going to get new LSMs spontaniously. If they don't it's up to > the "user". Unsophisticated users won't want this, and the others don't > need it. Hello, sorry if I missed something simple but I did not understand what "Automatic inclusions of new LSMs " and "get new LSMs spontaniously" is about. If I understood the kernel practice development correctly, when a new LSM will be included, it will have a dedicated "config SECURITY_MYNEWLSM" which will be default to "n" in order to respect the "principle of least astonishment". How could such a new LSM be automatically/spontaneously added to the LSM list? I understand that this is a tough issue and that the subject might have been discussed a few years ago, and if that's the case, it would be nice to have pointers to some clear documentation or past emails (and it would be very very nice if the kernel documentation was updated to document the current state of LSM stacking: for example https://www.kernel.org/doc/html/v5.11/admin-guide/LSM/index.html still documents the "security=" kernel parameter even though it conflicts with CONFIG_LSM and can be ignored by the kernel in practise). Thanks, Nicolas
Re: [PATCH v3 1/1] security: Add CONFIG_LSM_AUTO to handle default LSM stack ordering
On Mon, Feb 22, 2021 at 11:46 PM Casey Schaufler wrote: > > > On 2/22/2021 1:12 PM, Nicolas Iooss wrote: > > On Mon, Feb 22, 2021 at 9:32 PM Casey Schaufler > > wrote: > >> On 2/22/2021 10:31 AM, Mickaël Salaün wrote: > >>> On 22/02/2021 17:51, Casey Schaufler wrote: > >>>> On 2/22/2021 7:06 AM, Mickaël Salaün wrote: > >>>>> From: Mickaël Salaün > >>>>> > >>>>> Add a new option CONFIG_LSM_AUTO to enable users to delegate default LSM > >>>>> stacking order to kernel developers. This enable to keep a consistent > >>>>> order of enabled LSM when changing the LSM selection, especially when a > >>>>> new LSM is added to the kernel. > >>>> TL;DR - NAK > >>>> > >>>> Do you think that we might have considered this when stacking was > >>>> introduced? > >>> I didn't dig the detailed history of LSM stacking, but you are in Cc > >>> because I know that you know. I may have though that the main goal of > >>> the current LSM stacking implementation was to enable to stack existing > >>> LSMs, which works well with this CONFIG_LSM list, but doesn't work as > >>> well for new LSMs. > >> It works just fine for new LSMs if you treat them as significant > >> features which may have significant impact on the behavior of the > >> system. > >> > >>>> Did you even consider the implications before sending > >>>> the patch? > >>> Yes, and it doesn't change much the current behavior without user > >>> interaction. However, it gives the choice to users to choose how they > >>> want their configuration to evolve. > >> Automatic inclusions of new LSMs would be counter to existing practice. > >> It won't work for "major" LSMs. > >> > >> > >>>> This only makes any sense if you want to compile in > >>>> AppArmor and/or Smack but always use SELinux. The existing Kconfig > >>>> model handles that perfectly well. > >>> This patch series doesn't change this behavior if the user doesn't want > >>> it to change. > >> Well, there's the question. If a distribution/system uses the new scheme > >> "users" are going to get new LSMs spontaniously. If they don't it's up to > >> the "user". Unsophisticated users won't want this, and the others don't > >> need it. > > Hello, sorry if I missed something simple but I did not understand > > what "Automatic inclusions of new LSMs " and "get new LSMs > > spontaniously" is about. If I understood the kernel practice > > development correctly, when a new LSM will be included, it will have a > > dedicated "config SECURITY_MYNEWLSM" which will be default to "n" in > > order to respect the "principle of least astonishment". How could such > > a new LSM be automatically/spontaneously added to the LSM list? > > It wouldn't. But compiling the new LSM mynewlsm doesn't add it to > the list, either. Today no one should expect a LSM to be active if > it hasn't been added to the CONFIG_LSM list. The proposed addition > of CONFIG_LSM_AUTO would change that. "make oldconfig" would add > security modules that are built to the list. This is unnecessary > since whoever changed CONFIG_SECURITY_MYNEWLSM to "y" could easily > have added it to CONFIG_LSM. In the right place. > > > I understand that this is a tough issue and that the subject might > > have been discussed a few years ago, and if that's the case, it would > > be nice to have pointers to some clear documentation or past emails > > (and it would be very very nice if the kernel documentation was > > updated to document the current state of LSM stacking: > > I'm not going to argue against that. > > > for example > > https://www.kernel.org/doc/html/v5.11/admin-guide/LSM/index.html still > > documents the "security=" kernel parameter even though it conflicts > > with CONFIG_LSM and can be ignored by the kernel in practise). > > You can still select one "major" module using security= if you > don't use lsm= to specify a full list. We put real effort into > being backward compatible. No, this is not true. If CONFIG_LSM is defined to "lockdown,yama,bpf" and if the kernel command line contains "security=selinux" without any "lsm" parameter, then SELinux is not enabled prope
[PATCH] eventpoll: fix uninitialized variable in epoll_ctl
When calling epoll_ctl with operation EPOLL_CTL_DEL, structure epds is
not initialized but ep_take_care_of_epollwakeup reads its event field.
When this unintialized field has EPOLLWAKEUP bit set, a capability check
is done for CAP_BLOCK_SUSPEND in ep_take_care_of_epollwakeup. This
produces unexpected messages in the audit log, such as (on a system
running SELinux):
type=AVC msg=audit(1408212798.866:410): avc: denied
{ block_suspend } for pid=7754 comm="dbus-daemon" capability=36
scontext=unconfined_u:unconfined_r:unconfined_t
tcontext=unconfined_u:unconfined_r:unconfined_t
tclass=capability2 permissive=1
type=SYSCALL msg=audit(1408212798.866:410): arch=c03e syscall=233
success=yes exit=0 a0=3 a1=2 a2=9 a3=7fffd4d66ec0 items=0 ppid=1
pid=7754 auid=1000 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0
fsgid=0 tty=(none) ses=3 comm="dbus-daemon"
exe="/usr/bin/dbus-daemon"
subj=unconfined_u:unconfined_r:unconfined_t key=(null)
("arch=c03e syscall=233 a1=2" means "epoll_ctl(op=EPOLL_CTL_DEL)")
Remove use of epds in epoll_ctl when op == EPOLL_CTL_DEL.
Fixes: 4d7e30d98939 ("epoll: Add a flag, EPOLLWAKEUP, to prevent suspend
while epoll events are ready")
Signed-off-by: Nicolas Iooss
---
fs/eventpoll.c |3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index b10b48c..7bcfff9 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -1852,7 +1852,8 @@ SYSCALL_DEFINE4(epoll_ctl, int, epfd, int, op, int, fd,
goto error_tgt_fput;
/* Check if EPOLLWAKEUP is allowed */
- ep_take_care_of_epollwakeup(&epds);
+ if (ep_op_has_event(op))
+ ep_take_care_of_epollwakeup(&epds);
/*
* We have to check that the file structure underneath the file
descriptor
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 3/3] um/os-Linux: Use char[] for syscall_stub declarations
When declaring __syscall_stub_start, use the same type in UML userspace
code as in arch/um/include/asm/sections.h.
While at it, also declare batch_syscall_stub as char[].
Signed-off-by: Nicolas Iooss
---
arch/um/os-Linux/skas/mem.c | 6 +++---
arch/um/os-Linux/skas/process.c | 11 +--
2 files changed, 8 insertions(+), 9 deletions(-)
diff --git a/arch/um/os-Linux/skas/mem.c b/arch/um/os-Linux/skas/mem.c
index 689b18db798f..abb02becca80 100644
--- a/arch/um/os-Linux/skas/mem.c
+++ b/arch/um/os-Linux/skas/mem.c
@@ -19,7 +19,7 @@
#include
#include
-extern unsigned long batch_syscall_stub, __syscall_stub_start;
+extern char batch_syscall_stub[], __syscall_stub_start[];
extern void wait_stub_done(int pid);
@@ -39,8 +39,8 @@ static int __init init_syscall_regs(void)
{
get_safe_registers(syscall_regs, NULL);
syscall_regs[REGS_IP_INDEX] = STUB_CODE +
- ((unsigned long) &batch_syscall_stub -
-(unsigned long) &__syscall_stub_start);
+ ((unsigned long) batch_syscall_stub -
+(unsigned long) __syscall_stub_start);
return 0;
}
diff --git a/arch/um/os-Linux/skas/process.c b/arch/um/os-Linux/skas/process.c
index 908579f2b0ab..fa934d0c8932 100644
--- a/arch/um/os-Linux/skas/process.c
+++ b/arch/um/os-Linux/skas/process.c
@@ -193,7 +193,7 @@ static void handle_trap(int pid, struct uml_pt_regs *regs,
handle_syscall(regs);
}
-extern int __syscall_stub_start;
+extern char __syscall_stub_start[];
static int userspace_tramp(void *stack)
{
@@ -218,7 +218,7 @@ static int userspace_tramp(void *stack)
*/
int fd;
unsigned long long offset;
- fd = phys_mapping(to_phys(&__syscall_stub_start), &offset);
+ fd = phys_mapping(to_phys(__syscall_stub_start), &offset);
addr = mmap64((void *) STUB_CODE, UM_KERN_PAGE_SIZE,
PROT_EXEC, MAP_FIXED | MAP_PRIVATE, fd, offset);
if (addr == MAP_FAILED) {
@@ -245,7 +245,7 @@ static int userspace_tramp(void *stack)
unsigned long v = STUB_CODE +
(unsigned long) stub_segv_handler -
- (unsigned long) &__syscall_stub_start;
+ (unsigned long) __syscall_stub_start;
set_sigstack((void *) STUB_DATA, UM_KERN_PAGE_SIZE);
sigemptyset(&sa.sa_mask);
@@ -474,7 +474,7 @@ static int __init init_thread_regs(void)
/* Set parent's instruction pointer to start of clone-stub */
thread_regs[REGS_IP_INDEX] = STUB_CODE +
(unsigned long) stub_clone_handler -
- (unsigned long) &__syscall_stub_start;
+ (unsigned long) __syscall_stub_start;
thread_regs[REGS_SP_INDEX] = STUB_DATA + UM_KERN_PAGE_SIZE -
sizeof(void *);
#ifdef __SIGNAL_FRAMESIZE
@@ -582,8 +582,7 @@ int map_stub_pages(int fd, unsigned long code, unsigned
long data,
struct proc_mm_op mmop;
int n;
unsigned long long code_offset;
- int code_fd = phys_mapping(to_phys((void *) &__syscall_stub_start),
- &code_offset);
+ int code_fd = phys_mapping(to_phys(__syscall_stub_start), &code_offset);
mmop = ((struct proc_mm_op) { .op= MM_MMAP,
.u =
--
2.1.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/3] um: Create asm/sections.h
arch/um/kernel/dyn.lds.S and arch/um/kernel/uml.lds.S define some
UML-specific symbols. These symbols are used in the kernel part of UML
with extern declarations.
Move these declarations to a new header, asm/sections.h, like other
architectures do.
Signed-off-by: Nicolas Iooss
---
arch/um/include/asm/Kbuild | 1 -
arch/um/include/asm/sections.h | 9 +
arch/um/kernel/physmem.c | 3 +--
arch/um/kernel/skas/mmu.c | 3 +--
arch/um/kernel/um_arch.c | 2 --
5 files changed, 11 insertions(+), 7 deletions(-)
create mode 100644 arch/um/include/asm/sections.h
diff --git a/arch/um/include/asm/Kbuild b/arch/um/include/asm/Kbuild
index 244b12c8cb39..21fd5c647442 100644
--- a/arch/um/include/asm/Kbuild
+++ b/arch/um/include/asm/Kbuild
@@ -23,7 +23,6 @@ generic-y += pci.h
generic-y += percpu.h
generic-y += preempt.h
generic-y += scatterlist.h
-generic-y += sections.h
generic-y += switch_to.h
generic-y += topology.h
generic-y += trace_clock.h
diff --git a/arch/um/include/asm/sections.h b/arch/um/include/asm/sections.h
new file mode 100644
index ..3a6ebcc65519
--- /dev/null
+++ b/arch/um/include/asm/sections.h
@@ -0,0 +1,9 @@
+#ifndef __UM_SECTIONS_H
+#define __UM_SECTIONS_H
+
+#include
+
+extern char __binary_start;
+extern int __syscall_stub_start, __syscall_stub_end;
+
+#endif
diff --git a/arch/um/kernel/physmem.c b/arch/um/kernel/physmem.c
index 30fdd5d0067b..db05c067665a 100644
--- a/arch/um/kernel/physmem.c
+++ b/arch/um/kernel/physmem.c
@@ -8,6 +8,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -75,8 +76,6 @@ void map_memory(unsigned long virt, unsigned long phys,
unsigned long len,
}
}
-extern int __syscall_stub_start;
-
void __init setup_physmem(unsigned long start, unsigned long reserve_end,
unsigned long len, unsigned long long highmem)
{
diff --git a/arch/um/kernel/skas/mmu.c b/arch/um/kernel/skas/mmu.c
index 007d5503f49b..d2a0a4c0cd91 100644
--- a/arch/um/kernel/skas/mmu.c
+++ b/arch/um/kernel/skas/mmu.c
@@ -8,12 +8,11 @@
#include
#include
#include
+#include
#include
#include
#include
-extern int __syscall_stub_start;
-
static int init_stub_pte(struct mm_struct *mm, unsigned long proc,
unsigned long kernel)
{
diff --git a/arch/um/kernel/um_arch.c b/arch/um/kernel/um_arch.c
index 016adf0985d5..02c21f6e0983 100644
--- a/arch/um/kernel/um_arch.c
+++ b/arch/um/kernel/um_arch.c
@@ -259,8 +259,6 @@ EXPORT_SYMBOL(end_iomem);
#define MIN_VMALLOC (32 * 1024 * 1024)
-extern char __binary_start;
-
int __init linux_main(int argc, char **argv)
{
unsigned long avail, diff;
--
2.1.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/3] um: Use char[] for linker script address declarations
The linker script defines some variables which are declared either with type char[] in include/asm-generic/sections.h or with a meaningless integer type in arch/um/include/asm/sections.h. Fix this inconsistency by declaring every variable char[]. Signed-off-by: Nicolas Iooss --- arch/um/include/asm/sections.h | 4 ++-- arch/um/kernel/physmem.c | 4 ++-- arch/um/kernel/skas/mmu.c | 4 ++-- arch/um/kernel/um_arch.c | 2 +- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/arch/um/include/asm/sections.h b/arch/um/include/asm/sections.h index 3a6ebcc65519..cafcf684d947 100644 --- a/arch/um/include/asm/sections.h +++ b/arch/um/include/asm/sections.h @@ -3,7 +3,7 @@ #include -extern char __binary_start; -extern int __syscall_stub_start, __syscall_stub_end; +extern char __binary_start[]; +extern char __syscall_stub_start[], __syscall_stub_end[]; #endif diff --git a/arch/um/kernel/physmem.c b/arch/um/kernel/physmem.c index db05c067665a..c833a5dec1e7 100644 --- a/arch/um/kernel/physmem.c +++ b/arch/um/kernel/physmem.c @@ -100,8 +100,8 @@ void __init setup_physmem(unsigned long start, unsigned long reserve_end, * Special kludge - This page will be mapped in to userspace processes * from physmem_fd, so it needs to be written out there. */ - os_seek_file(physmem_fd, __pa(&__syscall_stub_start)); - os_write_file(physmem_fd, &__syscall_stub_start, PAGE_SIZE); + os_seek_file(physmem_fd, __pa(__syscall_stub_start)); + os_write_file(physmem_fd, __syscall_stub_start, PAGE_SIZE); os_fsync_file(physmem_fd); bootmap_size = init_bootmem(pfn, pfn + delta); diff --git a/arch/um/kernel/skas/mmu.c b/arch/um/kernel/skas/mmu.c index d2a0a4c0cd91..e40ce29bdc2c 100644 --- a/arch/um/kernel/skas/mmu.c +++ b/arch/um/kernel/skas/mmu.c @@ -108,7 +108,7 @@ void uml_setup_stubs(struct mm_struct *mm) return; ret = init_stub_pte(mm, STUB_CODE, - (unsigned long) &__syscall_stub_start); + (unsigned long) __syscall_stub_start); if (ret) goto out; @@ -116,7 +116,7 @@ void uml_setup_stubs(struct mm_struct *mm) if (ret) goto out; - mm->context.stub_pages[0] = virt_to_page(&__syscall_stub_start); + mm->context.stub_pages[0] = virt_to_page(__syscall_stub_start); mm->context.stub_pages[1] = virt_to_page(mm->context.id.stack); /* dup_mmap already holds mmap_sem */ diff --git a/arch/um/kernel/um_arch.c b/arch/um/kernel/um_arch.c index 02c21f6e0983..5dd632203dca 100644 --- a/arch/um/kernel/um_arch.c +++ b/arch/um/kernel/um_arch.c @@ -313,7 +313,7 @@ int __init linux_main(int argc, char **argv) physmem_size += UML_ROUND_UP(brk_start) - UML_ROUND_UP(&_end); } - uml_physmem = (unsigned long) &__binary_start & PAGE_MASK; + uml_physmem = (unsigned long) __binary_start & PAGE_MASK; /* Reserve up to 4M after the current brk */ uml_reserved = ROUND_4M(brk_start) + (1 << 22); -- 2.1.1 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] userns: simplify map_id_range_* functions
Functions map_id_range_down, map_id_down and map_id_up all used the construction: if (...) id = ... else id = ... return id; which can be simplified by directly returning the result of the computations in each branch. Moreover as the condition tested whether the "break;" in the previous for loop was hit, it is simpler to directly compute the result and return it. Signed-off-by: Nicolas Iooss --- As I could not find any relevant entry for kernel/user_namespace.c in MAINTAINERS, I have built the list of recipients from the git history. Please let me know if I was expected to proceed differently to submit this patch. kernel/user_namespace.c | 31 +++ 1 file changed, 7 insertions(+), 24 deletions(-) diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c index 4109f8320684..bf063dc6b8d4 100644 --- a/kernel/user_namespace.c +++ b/kernel/user_namespace.c @@ -165,15 +165,10 @@ static u32 map_id_range_down(struct uid_gid_map *map, u32 id, u32 count) last = first + map->extent[idx].count - 1; if (id >= first && id <= last && (id2 >= first && id2 <= last)) - break; + return (id - first) + map->extent[idx].lower_first; } - /* Map the id or note failure */ - if (idx < extents) - id = (id - first) + map->extent[idx].lower_first; - else - id = (u32) -1; - - return id; + /* Note failure */ + return (u32) -1; } static u32 map_id_down(struct uid_gid_map *map, u32 id) @@ -188,15 +183,9 @@ static u32 map_id_down(struct uid_gid_map *map, u32 id) first = map->extent[idx].first; last = first + map->extent[idx].count - 1; if (id >= first && id <= last) - break; + return (id - first) + map->extent[idx].lower_first; } - /* Map the id or note failure */ - if (idx < extents) - id = (id - first) + map->extent[idx].lower_first; - else - id = (u32) -1; - - return id; + return (u32) -1; } static u32 map_id_up(struct uid_gid_map *map, u32 id) @@ -211,15 +200,9 @@ static u32 map_id_up(struct uid_gid_map *map, u32 id) first = map->extent[idx].lower_first; last = first + map->extent[idx].count - 1; if (id >= first && id <= last) - break; + return (id - first) + map->extent[idx].first; } - /* Map the id or note failure */ - if (idx < extents) - id = (id - first) + map->extent[idx].first; - else - id = (u32) -1; - - return id; + return (u32) -1; } /** -- 2.4.6 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] userns: simplify map_id_range_* functions
On 07/27/2015 12:29 PM, Eric W. Biederman wrote: > Nicolas Iooss writes: > >> Functions map_id_range_down, map_id_down and map_id_up all used the >> construction: >> >> if (...) >> id = ... >> else >> id = ... >> return id; >> >> which can be simplified by directly returning the result of the >> computations in each branch. >> >> Moreover as the condition tested whether the "break;" in the previous >> for loop was hit, it is simpler to directly compute the result and >> return it. > > It is not a simplification, it is just code motion. I agree. Also on my system (x86_64 kernel with Arch Linux + CONFIG_USERNS configuration), the assembly code generated either by gcc 5.2.0 or by clang 3.6.2 (with LLVMLinux patches) does not change at all with this patch. So there would be absolutely no performance change or similar things which could motivate this change. > Further at least to my eyes adding multiple exit points and setting the > same value in two different places actually obscures what the functions > are doing. I did not understand what "setting the same value in two different places" refers to. Anyway, all right. I haven't got much experience about code maintenance so if you say my patch make things less clear, I believe you. > If we could talk about speeding up the performance of the stat system > call I think there would be a point in mucking with these functions. > > As it is I think it is I think merging your patch will just make it more > difficult to understand what the code is doing in the future, with no > benefit except a reduction in line count. OK. My intention was precisely to make the code easier to understand (because I spent some time before I understood there really were only two cases: existing mapping or not) but if you think my patch does the opposite, I accept dropping it. Thanks for your comments. Nicolas -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] fixdep: constify strrcmp arguments
Hello,
I sent the path below a few weeks ago and did not have any feedback.
Could you please review it?
Thanks,
Nicolas
On 11/18/2015 07:07 PM, Nicolas Iooss wrote:
> strrcmp only performs read access to the memory addressed by its
> arguments so make them const pointers.
>
> Signed-off-by: Nicolas Iooss
> ---
> scripts/basic/fixdep.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/scripts/basic/fixdep.c b/scripts/basic/fixdep.c
> index c68fd61fdc42..5b327c67a828 100644
> --- a/scripts/basic/fixdep.c
> +++ b/scripts/basic/fixdep.c
> @@ -251,7 +251,7 @@ static void parse_config_file(const char *map, size_t len)
> }
>
> /* test is s ends in sub */
> -static int strrcmp(char *s, char *sub)
> +static int strrcmp(const char *s, const char *sub)
> {
> int slen = strlen(s);
> int sublen = strlen(sub);
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drm: do not use device name as a format string
Hello, I sent the path below a few weeks ago and did not have any feedback. Is there any issue in it that I need to fix before submitting it again? Thanks, Nicolas Iooss On 11/18/2015 06:58 PM, Nicolas Iooss wrote: > drm_dev_set_unique() formats its parameter using kvasprintf() but many > of its callers directly pass dev_name(dev) as printf format string, > without any format parameter. This can cause some issues when the > device name contains '%' characters. > > To avoid any potential issue, always use "%s" when using > drm_dev_set_unique() with dev_name(). > > Signed-off-by: Nicolas Iooss > --- > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 2 +- > drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c| 2 +- > drivers/gpu/drm/tegra/drm.c | 2 +- > drivers/gpu/drm/vc4/vc4_drv.c| 2 +- > include/drm/drmP.h | 1 + > 5 files changed, 5 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c > b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c > index 244df0a440b7..0d720d3a7ee0 100644 > --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c > +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c > @@ -733,7 +733,7 @@ static int atmel_hlcdc_dc_drm_probe(struct > platform_device *pdev) > if (!ddev) > return -ENOMEM; > > - ret = drm_dev_set_unique(ddev, dev_name(ddev->dev)); > + ret = drm_dev_set_unique(ddev, "%s", dev_name(ddev->dev)); > if (ret) > goto err_unref; > > diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c > b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c > index 1930234ba5f1..947d75f59881 100644 > --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c > +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c > @@ -363,7 +363,7 @@ static int fsl_dcu_drm_probe(struct platform_device *pdev) > fsl_dev->np = dev->of_node; > drm->dev_private = fsl_dev; > dev_set_drvdata(dev, fsl_dev); > - drm_dev_set_unique(drm, dev_name(dev)); > + drm_dev_set_unique(drm, "%s", dev_name(dev)); > > ret = drm_dev_register(drm, 0); > if (ret < 0) > diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c > index 159ef515cab1..b278f60f4376 100644 > --- a/drivers/gpu/drm/tegra/drm.c > +++ b/drivers/gpu/drm/tegra/drm.c > @@ -991,7 +991,7 @@ static int host1x_drm_probe(struct host1x_device *dev) > if (!drm) > return -ENOMEM; > > - drm_dev_set_unique(drm, dev_name(&dev->dev)); > + drm_dev_set_unique(drm, "%s", dev_name(&dev->dev)); > dev_set_drvdata(&dev->dev, drm); > > err = drm_dev_register(drm, 0); > diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c > index 6e730605edcc..c90a451aaa79 100644 > --- a/drivers/gpu/drm/vc4/vc4_drv.c > +++ b/drivers/gpu/drm/vc4/vc4_drv.c > @@ -168,7 +168,7 @@ static int vc4_drm_bind(struct device *dev) > vc4->dev = drm; > drm->dev_private = vc4; > > - drm_dev_set_unique(drm, dev_name(dev)); > + drm_dev_set_unique(drm, "%s", dev_name(dev)); > > drm_mode_config_init(drm); > if (ret) > diff --git a/include/drm/drmP.h b/include/drm/drmP.h > index 0b921ae06cd8..995fb96cb740 100644 > --- a/include/drm/drmP.h > +++ b/include/drm/drmP.h > @@ -1049,6 +1049,7 @@ void drm_dev_ref(struct drm_device *dev); > void drm_dev_unref(struct drm_device *dev); > int drm_dev_register(struct drm_device *dev, unsigned long flags); > void drm_dev_unregister(struct drm_device *dev); > +__printf(2, 3) > int drm_dev_set_unique(struct drm_device *dev, const char *fmt, ...); > > struct drm_minor *drm_minor_acquire(unsigned int minor_id); > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drm: do not use device name as a format string
On 12/06/2015 10:35 AM, Daniel Vetter wrote:
>> On 11/18/2015 06:58 PM, Nicolas Iooss wrote:
>>> drm_dev_set_unique() formats its parameter using kvasprintf() but many
>>> of its callers directly pass dev_name(dev) as printf format string,
>>> without any format parameter. This can cause some issues when the
>>> device name contains '%' characters.
>>>
>>> To avoid any potential issue, always use "%s" when using
>>> drm_dev_set_unique() with dev_name().
>
> Not sure this is worth it really, normally people don't place % characters
> into their device names, ever. And if they do it'll blow up. There's also
> no security issue here since userspace can't set this name.
>
> If the maintainers of the affected drivers don't want this I won't merge
> this patch.
Actually I had the same opinion before I began to add __printf
attributes and "%s" in several places in the kernel to make
-Wformat-security useful. This led me to discover some funny issues
like the one fixed by commit 3958b79266b1 ("configfs: fix kernel
infoleak through user-controlled format string",
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=3958b79266b14729edd61daf9dfb84de45f4ec6d
). The patch I sent is in fact a very small step towards making
-Wformat-security useful again to detect "real" issues.
Of course, if you do not feel it is worth it and believe that dev_name
is fully controlled by trusted sources which will never introduce any %
character, I understand your will of not merging my patch.
Regards,
Nicolas
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drm: do not use device name as a format string
On 12/07/2015 01:31 PM, Thierry Reding wrote:
> On Mon, Dec 07, 2015 at 12:46:52PM +0100, Daniel Vetter wrote:
>> On Mon, Dec 07, 2015 at 11:53:01AM +0200, Jani Nikula wrote:
>>> On Mon, 07 Dec 2015, Daniel Vetter wrote:
>>>> On Sun, Dec 06, 2015 at 11:16:32AM +0100, Nicolas Iooss wrote:
>>>>> On 12/06/2015 10:35 AM, Daniel Vetter wrote:
>>>>>>> On 11/18/2015 06:58 PM, Nicolas Iooss wrote:
>>>>>>>> drm_dev_set_unique() formats its parameter using kvasprintf() but many
>>>>>>>> of its callers directly pass dev_name(dev) as printf format string,
>>>>>>>> without any format parameter. This can cause some issues when the
>>>>>>>> device name contains '%' characters.
>>>>>>>>
>>>>>>>> To avoid any potential issue, always use "%s" when using
>>>>>>>> drm_dev_set_unique() with dev_name().
>>>>>>
>>>>>> Not sure this is worth it really, normally people don't place %
>>>>>> characters
>>>>>> into their device names, ever. And if they do it'll blow up. There's also
>>>>>> no security issue here since userspace can't set this name.
>>>>>>
>>>>>> If the maintainers of the affected drivers don't want this I won't merge
>>>>>> this patch.
>>>>>
>>>>> Actually I had the same opinion before I began to add __printf
>>>>> attributes and "%s" in several places in the kernel to make
>>>>> -Wformat-security useful. This led me to discover some funny issues
>>>>> like the one fixed by commit 3958b79266b1 ("configfs: fix kernel
>>>>> infoleak through user-controlled format string",
>>>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=3958b79266b14729edd61daf9dfb84de45f4ec6d
>>>>> ). The patch I sent is in fact a very small step towards making
>>>>> -Wformat-security useful again to detect "real" issues.
>>>>>
>>>>> Of course, if you do not feel it is worth it and believe that dev_name
>>>>> is fully controlled by trusted sources which will never introduce any %
>>>>> character, I understand your will of not merging my patch.
>>>>
>>>> Ah, that's the context I was missing, that really should be in the commit
>>>> message. If this is part of an overall effort to enable something useful
>>>> it makes more sense to get it in.
>>>>
>>>> On the patch itself it feels rather funny to do a "%s", str); combo, maybe
>>>> we should have a drm_dev_set_unique_static instead? Including kerneldoc
>>>> explaining why there's too.
>>>
>>> No caller of drm_dev_set_unique() actually uses the formatting for
>>> anything... so you'd end up with drm_dev_set_unique_static() and an
>>> orphaned drm_dev_set_unique()...
>>
>> Ok, then I guess we can just ditch the printf stuff from set_unique.
>> Nicolas, you're up for that?
>
> Looking at all the callsites of drm_dev_set_unique() it seems like all
> of the drivers (with the exception of vgem) use dev_name() on the same
> device that's already passed into drm_dev_alloc(), so perhaps another
> alternative would be to have drm_dev_alloc() set the unique name by
> default and keep the function for cases where it needs to be set
> explicitly (like for vgem). vgem passes drm_dev_alloc() a NULL device,
> so that could serve as condition.
I have written a patch which removes the printf format processing from
drm_dev_set_unique(). I will test it as soon as possible and depending
on the results, send it or explain what went wrong. If no one does the
work before me, I'll also take a look at calling drm_dev_set_unique() in
drm_dev_alloc(), and this would be an other patch.
Thanks,
Nicolas
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] drm: do not use device name as a format string
drm_dev_set_unique() formats its parameter using kvasprintf() but many of its callers directly pass dev_name(dev) as printf format string, without any format parameter. This can cause some issues when the device name contains '%' characters. To avoid any potential issue, always use "%s" when using drm_dev_set_unique() with dev_name(). Signed-off-by: Nicolas Iooss --- drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 2 +- drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c| 2 +- drivers/gpu/drm/tegra/drm.c | 2 +- drivers/gpu/drm/vc4/vc4_drv.c| 2 +- include/drm/drmP.h | 1 + 5 files changed, 5 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c index 244df0a440b7..0d720d3a7ee0 100644 --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c @@ -733,7 +733,7 @@ static int atmel_hlcdc_dc_drm_probe(struct platform_device *pdev) if (!ddev) return -ENOMEM; - ret = drm_dev_set_unique(ddev, dev_name(ddev->dev)); + ret = drm_dev_set_unique(ddev, "%s", dev_name(ddev->dev)); if (ret) goto err_unref; diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c index 1930234ba5f1..947d75f59881 100644 --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c @@ -363,7 +363,7 @@ static int fsl_dcu_drm_probe(struct platform_device *pdev) fsl_dev->np = dev->of_node; drm->dev_private = fsl_dev; dev_set_drvdata(dev, fsl_dev); - drm_dev_set_unique(drm, dev_name(dev)); + drm_dev_set_unique(drm, "%s", dev_name(dev)); ret = drm_dev_register(drm, 0); if (ret < 0) diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c index 159ef515cab1..b278f60f4376 100644 --- a/drivers/gpu/drm/tegra/drm.c +++ b/drivers/gpu/drm/tegra/drm.c @@ -991,7 +991,7 @@ static int host1x_drm_probe(struct host1x_device *dev) if (!drm) return -ENOMEM; - drm_dev_set_unique(drm, dev_name(&dev->dev)); + drm_dev_set_unique(drm, "%s", dev_name(&dev->dev)); dev_set_drvdata(&dev->dev, drm); err = drm_dev_register(drm, 0); diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c index 6e730605edcc..c90a451aaa79 100644 --- a/drivers/gpu/drm/vc4/vc4_drv.c +++ b/drivers/gpu/drm/vc4/vc4_drv.c @@ -168,7 +168,7 @@ static int vc4_drm_bind(struct device *dev) vc4->dev = drm; drm->dev_private = vc4; - drm_dev_set_unique(drm, dev_name(dev)); + drm_dev_set_unique(drm, "%s", dev_name(dev)); drm_mode_config_init(drm); if (ret) diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 0b921ae06cd8..995fb96cb740 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1049,6 +1049,7 @@ void drm_dev_ref(struct drm_device *dev); void drm_dev_unref(struct drm_device *dev); int drm_dev_register(struct drm_device *dev, unsigned long flags); void drm_dev_unregister(struct drm_device *dev); +__printf(2, 3) int drm_dev_set_unique(struct drm_device *dev, const char *fmt, ...); struct drm_minor *drm_minor_acquire(unsigned int minor_id); -- 2.6.2 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] fixdep: constify strrcmp arguments
strrcmp only performs read access to the memory addressed by its
arguments so make them const pointers.
Signed-off-by: Nicolas Iooss
---
scripts/basic/fixdep.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/basic/fixdep.c b/scripts/basic/fixdep.c
index c68fd61fdc42..5b327c67a828 100644
--- a/scripts/basic/fixdep.c
+++ b/scripts/basic/fixdep.c
@@ -251,7 +251,7 @@ static void parse_config_file(const char *map, size_t len)
}
/* test is s ends in sub */
-static int strrcmp(char *s, char *sub)
+static int strrcmp(const char *s, const char *sub)
{
int slen = strlen(s);
int sublen = strlen(sub);
--
2.6.2
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] iwlwifi: mvm: fix tof.h header guard
Commit ce7929186a39 ("iwlwifi: mvm: add basic Time of Flight (802.11mc
FTM) support") created drivers/net/wireless/iwlwifi/mvm/tof.h with a
broken header guard:
#ifndef __tof
#define __tof_h__
...
#endif /* __tof_h__ */
Use __tof_h__ in the first line.
Signed-off-by: Nicolas Iooss
---
drivers/net/wireless/iwlwifi/mvm/tof.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/iwlwifi/mvm/tof.h
b/drivers/net/wireless/iwlwifi/mvm/tof.h
index 50ae8adaaa6e..9beebc33cb8d 100644
--- a/drivers/net/wireless/iwlwifi/mvm/tof.h
+++ b/drivers/net/wireless/iwlwifi/mvm/tof.h
@@ -60,7 +60,7 @@
* OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*
*/
-#ifndef __tof
+#ifndef __tof_h__
#define __tof_h__
#include "fw-api-tof.h"
--
2.5.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/2] drm: use dev_name as default unique name in drm_dev_alloc()
The following code pattern exists in some DRM drivers:
ddev = drm_dev_alloc(&driver, parent_dev);
drm_dev_set_unique(ddev, dev_name(parent_dev));
(Sometimes dev_name(ddev->dev) is used, which is the same.)
As suggested in
http://lists.freedesktop.org/archives/dri-devel/2015-December/096441.html,
the unique name of a new DRM device can be set as dev_name(parent_dev)
when parent_dev is not NULL (vgem is a special case).
Signed-off-by: Nicolas Iooss
---
drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 4
drivers/gpu/drm/drm_drv.c| 9 +
drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c| 1 -
drivers/gpu/drm/nouveau/nouveau_drm.c| 4
drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 4
drivers/gpu/drm/tegra/drm.c | 1 -
drivers/gpu/drm/vc4/vc4_drv.c| 2 --
7 files changed, 9 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
index 244df0a440b7..fba4f72e7ae1 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
@@ -733,10 +733,6 @@ static int atmel_hlcdc_dc_drm_probe(struct platform_device
*pdev)
if (!ddev)
return -ENOMEM;
- ret = drm_dev_set_unique(ddev, dev_name(ddev->dev));
- if (ret)
- goto err_unref;
-
ret = atmel_hlcdc_dc_load(ddev);
if (ret)
goto err_unref;
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 20eaa0aae205..df749a6156e0 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -633,8 +633,17 @@ struct drm_device *drm_dev_alloc(struct drm_driver *driver,
}
}
+ if (parent) {
+ ret = drm_dev_set_unique(dev, dev_name(parent));
+ if (ret)
+ goto err_setunique;
+ }
+
return dev;
+err_setunique:
+ if (drm_core_check_feature(dev, DRIVER_GEM))
+ drm_gem_destroy(dev);
err_ctxbitmap:
drm_legacy_ctxbitmap_cleanup(dev);
drm_ht_remove(&dev->map_hash);
diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
index 1930234ba5f1..fca97d3fc846 100644
--- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
+++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
@@ -363,7 +363,6 @@ static int fsl_dcu_drm_probe(struct platform_device *pdev)
fsl_dev->np = dev->of_node;
drm->dev_private = fsl_dev;
dev_set_drvdata(dev, fsl_dev);
- drm_dev_set_unique(drm, dev_name(dev));
ret = drm_dev_register(drm, 0);
if (ret < 0)
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c
b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 2d23f95f17ce..b3a563c44bcd 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -1046,10 +1046,6 @@ nouveau_platform_device_create(const struct
nvkm_device_tegra_func *func,
goto err_free;
}
- err = drm_dev_set_unique(drm, dev_name(&pdev->dev));
- if (err < 0)
- goto err_free;
-
drm->platformdev = pdev;
platform_set_drvdata(pdev, drm);
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
index 215d6c44af55..afbb7407c44f 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
@@ -450,10 +450,6 @@ static int rockchip_drm_bind(struct device *dev)
if (!drm)
return -ENOMEM;
- ret = drm_dev_set_unique(drm, dev_name(dev));
- if (ret)
- goto err_free;
-
ret = drm_dev_register(drm, 0);
if (ret)
goto err_free;
diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index 159ef515cab1..12e2d3ccbc9d 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -991,7 +991,6 @@ static int host1x_drm_probe(struct host1x_device *dev)
if (!drm)
return -ENOMEM;
- drm_dev_set_unique(drm, dev_name(&dev->dev));
dev_set_drvdata(&dev->dev, drm);
err = drm_dev_register(drm, 0);
diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
index d5db9e0f3b73..647772305e8f 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.c
+++ b/drivers/gpu/drm/vc4/vc4_drv.c
@@ -168,8 +168,6 @@ static int vc4_drm_bind(struct device *dev)
vc4->dev = drm;
drm->dev_private = vc4;
- drm_dev_set_unique(drm, dev_name(dev));
-
drm_mode_config_init(drm);
if (ret)
goto unref;
--
2.6.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/2] drm: make drm_dev_set_unique() not use a format string
drm_dev_set_unique() uses a format string to define the unique name of a
device. This feature is not used as currently all the calls to this
function either use "%s" as a format string or directly use
dev_name().
Even though this second kind of call does not introduce security
problems, because there cannot be "%" characters in dev_name() results,
gcc issues a warning when building with -Wformat-security flag
("warning: format string is not a string literal (potentially
insecure)"). This warning is useful to find real bugs like the one
fixed by commit 3958b79266b1 ("configfs: fix kernel infoleak through
user-controlled format string"). False positives which do not bring
an extra value make the work of finding real bugs harder.
Therefore remove the format-string feature from drm_dev_set_unique().
Signed-off-by: Nicolas Iooss
---
drivers/gpu/drm/drm_drv.c | 11 +++
drivers/gpu/drm/nouveau/nouveau_drm.c | 2 +-
drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 2 +-
include/drm/drmP.h | 2 +-
4 files changed, 6 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 7dd6728dd092..20eaa0aae205 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -797,7 +797,7 @@ EXPORT_SYMBOL(drm_dev_unregister);
/**
* drm_dev_set_unique - Set the unique name of a DRM device
* @dev: device of which to set the unique name
- * @fmt: format string for unique name
+ * @name: unique name
*
* Sets the unique name of a DRM device using the specified format string and
* a variable list of arguments. Drivers can use this at driver probe time if
@@ -805,15 +805,10 @@ EXPORT_SYMBOL(drm_dev_unregister);
*
* Return: 0 on success or a negative error code on failure.
*/
-int drm_dev_set_unique(struct drm_device *dev, const char *fmt, ...)
+int drm_dev_set_unique(struct drm_device *dev, const char *name)
{
- va_list ap;
-
kfree(dev->unique);
-
- va_start(ap, fmt);
- dev->unique = kvasprintf(GFP_KERNEL, fmt, ap);
- va_end(ap);
+ dev->unique = kstrdup(name, GFP_KERNEL);
return dev->unique ? 0 : -ENOMEM;
}
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c
b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 1d3ee5179ab8..2d23f95f17ce 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -1046,7 +1046,7 @@ nouveau_platform_device_create(const struct
nvkm_device_tegra_func *func,
goto err_free;
}
- err = drm_dev_set_unique(drm, "%s", dev_name(&pdev->dev));
+ err = drm_dev_set_unique(drm, dev_name(&pdev->dev));
if (err < 0)
goto err_free;
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
index f22e1e1ee64a..215d6c44af55 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
@@ -450,7 +450,7 @@ static int rockchip_drm_bind(struct device *dev)
if (!drm)
return -ENOMEM;
- ret = drm_dev_set_unique(drm, "%s", dev_name(dev));
+ ret = drm_dev_set_unique(drm, dev_name(dev));
if (ret)
goto err_free;
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 0a271ca1f7c7..f14c25a6bbf2 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1059,7 +1059,7 @@ void drm_dev_ref(struct drm_device *dev);
void drm_dev_unref(struct drm_device *dev);
int drm_dev_register(struct drm_device *dev, unsigned long flags);
void drm_dev_unregister(struct drm_device *dev);
-int drm_dev_set_unique(struct drm_device *dev, const char *fmt, ...);
+int drm_dev_set_unique(struct drm_device *dev, const char *name);
struct drm_minor *drm_minor_acquire(unsigned int minor_id);
void drm_minor_release(struct drm_minor *minor);
--
2.6.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] drm: make drm_dev_set_unique() not use a format string
On 12/09/2015 12:28 AM, Emil Velikov wrote:
> On 8 December 2015 at 22:12, Nicolas Iooss
> wrote:
>> drm_dev_set_unique() uses a format string to define the unique name of a
>> device. This feature is not used as currently all the calls to this
>> function either use "%s" as a format string or directly use
>> dev_name().
>>
>> Even though this second kind of call does not introduce security
>> problems, because there cannot be "%" characters in dev_name() results,
>> gcc issues a warning when building with -Wformat-security flag
>> ("warning: format string is not a string literal (potentially
>> insecure)"). This warning is useful to find real bugs like the one
>> fixed by commit 3958b79266b1 ("configfs: fix kernel infoleak through
>> user-controlled format string"). False positives which do not bring
>> an extra value make the work of finding real bugs harder.
>>
>> Therefore remove the format-string feature from drm_dev_set_unique().
>>
>> Signed-off-by: Nicolas Iooss
>> ---
>> drivers/gpu/drm/drm_drv.c | 11 +++
>> drivers/gpu/drm/nouveau/nouveau_drm.c | 2 +-
>> drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 2 +-
>> include/drm/drmP.h | 2 +-
>> 4 files changed, 6 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>> index 7dd6728dd092..20eaa0aae205 100644
>> --- a/drivers/gpu/drm/drm_drv.c
>> +++ b/drivers/gpu/drm/drm_drv.c
>> @@ -797,7 +797,7 @@ EXPORT_SYMBOL(drm_dev_unregister);
>> /**
>> * drm_dev_set_unique - Set the unique name of a DRM device
>> * @dev: device of which to set the unique name
>> - * @fmt: format string for unique name
>> + * @name: unique name
>> *
>> * Sets the unique name of a DRM device using the specified format string
>> and
>> * a variable list of arguments. Drivers can use this at driver probe time
>> if
> You might want to also update the above hunk :-)
Indeed, thanks! I will wait a little bit for other feedbacks, read all
the comments/documentation to see if anything else needs an update and
submit a v2.
Nicolas
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] x86/boot: remove unused is_big_kernel variable
Variable is_big_kernel is defined in arch/x86/boot/tools/build.c but never used anywhere. Signed-off-by: Nicolas Iooss --- arch/x86/boot/tools/build.c | 1 - 1 file changed, 1 deletion(-) diff --git a/arch/x86/boot/tools/build.c b/arch/x86/boot/tools/build.c index a7661c430cd9..0702d2531bc7 100644 --- a/arch/x86/boot/tools/build.c +++ b/arch/x86/boot/tools/build.c @@ -49,7 +49,6 @@ typedef unsigned int u32; /* This must be large enough to hold the entire setup */ u8 buf[SETUP_SECT_MAX*512]; -int is_big_kernel; #define PECOFF_RELOC_RESERVE 0x20 -- 2.7.1
[PATCH] um: always use the same type for __syscall_stub_start
syscall_stub_start is declared with different types in C files:
arch/um/kernel/physmem.c: extern int __syscall_stub_start;
arch/um/kernel/skas/mmu.c: extern int __syscall_stub_start;
arch/um/os-Linux/skas/mem.c: extern unsigned long __syscall_stub_start;
arch/um/os-Linux/skas/process.c: extern int __syscall_stub_start;
Fix this inconsistency by always using unsigned long. This does not
change anything in the compiled code because only the address of
__syscall_stub_start is used, but it makes the static checker I use
stop complaining about incompatible declarations.
Signed-off-by: Nicolas Iooss
---
arch/um/kernel/physmem.c| 2 +-
arch/um/kernel/skas/mmu.c | 2 +-
arch/um/os-Linux/skas/process.c | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/um/kernel/physmem.c b/arch/um/kernel/physmem.c
index 30fdd5d0067b..f1d7ed26d638 100644
--- a/arch/um/kernel/physmem.c
+++ b/arch/um/kernel/physmem.c
@@ -75,7 +75,7 @@ void map_memory(unsigned long virt, unsigned long phys,
unsigned long len,
}
}
-extern int __syscall_stub_start;
+extern unsigned long __syscall_stub_start;
void __init setup_physmem(unsigned long start, unsigned long reserve_end,
unsigned long len, unsigned long long highmem)
diff --git a/arch/um/kernel/skas/mmu.c b/arch/um/kernel/skas/mmu.c
index 007d5503f49b..4bdd49e0bdc3 100644
--- a/arch/um/kernel/skas/mmu.c
+++ b/arch/um/kernel/skas/mmu.c
@@ -12,7 +12,7 @@
#include
#include
-extern int __syscall_stub_start;
+extern unsigned long __syscall_stub_start;
static int init_stub_pte(struct mm_struct *mm, unsigned long proc,
unsigned long kernel)
diff --git a/arch/um/os-Linux/skas/process.c b/arch/um/os-Linux/skas/process.c
index 908579f2b0ab..f30575557791 100644
--- a/arch/um/os-Linux/skas/process.c
+++ b/arch/um/os-Linux/skas/process.c
@@ -193,7 +193,7 @@ static void handle_trap(int pid, struct uml_pt_regs *regs,
handle_syscall(regs);
}
-extern int __syscall_stub_start;
+extern unsigned long __syscall_stub_start;
static int userspace_tramp(void *stack)
{
--
2.1.2
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] um: always use the same type for __syscall_stub_start
2014-10-11 13:42 GMT+02:00 Richard Weinberger: > Am 11.10.2014 um 13:29 schrieb Nicolas Iooss: >> syscall_stub_start is declared with different types in C files: >> >> arch/um/kernel/physmem.c: extern int __syscall_stub_start; >> arch/um/kernel/skas/mmu.c: extern int __syscall_stub_start; >> arch/um/os-Linux/skas/mem.c: extern unsigned long __syscall_stub_start; >> arch/um/os-Linux/skas/process.c: extern int __syscall_stub_start; >> >> Fix this inconsistency by always using unsigned long. This does not >> change anything in the compiled code because only the address of >> __syscall_stub_start is used, but it makes the static checker I use >> stop complaining about incompatible declarations. > > While we're here, can you put these declarations into a single header file? Sure. Do you have a specific header file in mind or shall I create arch/um/include/asm/sections.h with declarations for __syscall_stub_start, __syscall_stub_end and __binary_start (used in arch/um/kernel/um_arch.c)? Thanks for your quick reply, Nicolas -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] um: always use the same type for __syscall_stub_start
Le 11/10/2014 14:24, Richard Weinberger a écrit : > Am 11.10.2014 um 14:15 schrieb Nicolas Iooss: >> 2014-10-11 13:42 GMT+02:00 Richard Weinberger: >>> Am 11.10.2014 um 13:29 schrieb Nicolas Iooss: >>>> syscall_stub_start is declared with different types in C files: >>>> >>>> arch/um/kernel/physmem.c: extern int __syscall_stub_start; >>>> arch/um/kernel/skas/mmu.c: extern int __syscall_stub_start; >>>> arch/um/os-Linux/skas/mem.c: extern unsigned long __syscall_stub_start; >>>> arch/um/os-Linux/skas/process.c: extern int __syscall_stub_start; >>>> >>>> Fix this inconsistency by always using unsigned long. This does not >>>> change anything in the compiled code because only the address of >>>> __syscall_stub_start is used, but it makes the static checker I use >>>> stop complaining about incompatible declarations. >>> >>> While we're here, can you put these declarations into a single header >> file? >> >> Sure. Do you have a specific header file in mind or shall I create >> arch/um/include/asm/sections.h with declarations for >> __syscall_stub_start, __syscall_stub_end and __binary_start (used in >> arch/um/kernel/um_arch.c)? > > Not really. Maybe you can find a common header for all. > But I fear where is a reason why these declarations are not in a > common header. They are used in the kernel- and userspace part of > UML. > Anyway, please give it a try. :) Ok. I'll at least try to add a kernel header and send a new patch after some tests. By the way, most variables in include/asm-generic/sections.h are declared "char[]" and used without operator, contrary to __syscall_stub_start, declared "int" or "unsigned long" and only used with "&" operator. Is there any reason why there are in the kernel two ways of declaring/accessing code addresses defined in linker files? If not, I can send a patch which makes __syscall_stub_start "char[]" instead of "unsigned long" to make the code a little bit clearer. Nicolas -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2 2/2] coredump: add __printf attribute to cn_*printf functions
This allows detecting improper format string at build time, like:
fs/coredump.c:225:5: warning: format '%ld' expects argument of type
'long int', but argument 3 has type 'int' [-Wformat=]
err = cn_printf(cn, "%ld", cprm->siginfo->si_signo);
^
As si_signo is always an int, the format should be %d here.
Signed-off-by: Nicolas Iooss
---
fs/coredump.c | 11 +++
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/fs/coredump.c b/fs/coredump.c
index 833a57bc856c..e52e0064feac 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -70,7 +70,8 @@ static int expand_corename(struct core_name *cn, int size)
return 0;
}
-static int cn_vprintf(struct core_name *cn, const char *fmt, va_list arg)
+static __printf(2, 0) int cn_vprintf(struct core_name *cn, const char *fmt,
+va_list arg)
{
int free, need;
va_list arg_copy;
@@ -93,7 +94,7 @@ again:
return -ENOMEM;
}
-static int cn_printf(struct core_name *cn, const char *fmt, ...)
+static __printf(2, 3) int cn_printf(struct core_name *cn, const char *fmt, ...)
{
va_list arg;
int ret;
@@ -105,7 +106,8 @@ static int cn_printf(struct core_name *cn, const char *fmt,
...)
return ret;
}
-static int cn_esc_printf(struct core_name *cn, const char *fmt, ...)
+static __printf(2, 3)
+int cn_esc_printf(struct core_name *cn, const char *fmt, ...)
{
int cur = cn->used;
va_list arg;
@@ -225,7 +227,8 @@ static int format_corename(struct core_name *cn, struct
coredump_params *cprm)
break;
/* signal that caused the coredump */
case 's':
- err = cn_printf(cn, "%ld",
cprm->siginfo->si_signo);
+ err = cn_printf(cn, "%d",
+ cprm->siginfo->si_signo);
break;
/* UNIX time of coredump */
case 't': {
--
2.4.0
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2 1/2] coredump: use from_kuid/kgid when formatting corename
When adding __printf attribute to cn_printf, gcc reports some issues: fs/coredump.c:213:5: warning: format '%d' expects argument of type 'int', but argument 3 has type 'kuid_t' [-Wformat=] err = cn_printf(cn, "%d", cred->uid); ^ fs/coredump.c:217:5: warning: format '%d' expects argument of type 'int', but argument 3 has type 'kgid_t' [-Wformat=] err = cn_printf(cn, "%d", cred->gid); ^ These warnings come from the fact that the value of uid/gid needs to be extracted from the kuid_t/kgid_t structure before being used as an integer. More precisely, cred->uid and cred->gid need to be converted to either user-namespace uid/gid or to init_user_ns uid/gid. Use init_user_ns in order not to break existing ABI, and document this in Documentation/sysctl/kernel.txt. While at it, format uid and gid values with %u instead of %d because uid_t/__kernel_uid32_t and gid_t/__kernel_gid32_t are unsigned int. Signed-off-by: Nicolas Iooss --- Documentation/sysctl/kernel.txt | 4 ++-- fs/coredump.c | 8 ++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt index c831001c45f1..e1913f78f21e 100644 --- a/Documentation/sysctl/kernel.txt +++ b/Documentation/sysctl/kernel.txt @@ -197,8 +197,8 @@ core_pattern is used to specify a core dumpfile pattern name. %P global pid (init PID namespace) %i tid %I global tid (init PID namespace) - %u uid - %g gid + %u uid (in initial user namespace) + %g gid (in initial user namespace) %d dump mode, matches PR_SET_DUMPABLE and /proc/sys/fs/suid_dumpable %s signal number diff --git a/fs/coredump.c b/fs/coredump.c index bbbe139ab280..833a57bc856c 100644 --- a/fs/coredump.c +++ b/fs/coredump.c @@ -209,11 +209,15 @@ static int format_corename(struct core_name *cn, struct coredump_params *cprm) break; /* uid */ case 'u': - err = cn_printf(cn, "%d", cred->uid); + err = cn_printf(cn, "%u", + from_kuid(&init_user_ns, + cred->uid)); break; /* gid */ case 'g': - err = cn_printf(cn, "%d", cred->gid); + err = cn_printf(cn, "%u", + from_kgid(&init_user_ns, + cred->gid)); break; case 'd': err = cn_printf(cn, "%d", -- 2.4.0 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] rcu: declare rcu_data variables in the section they are defined in
On 05/05/2015 04:33 AM, Paul E. McKenney wrote:
> On Sun, May 03, 2015 at 12:27:02PM -0700, Josh Triplett wrote:
>> On Sun, May 03, 2015 at 05:57:53PM +0800, Nicolas Iooss wrote:
>>> Commit 11bbb235c26f ("rcu: Use DEFINE_PER_CPU_SHARED_ALIGNED for
>>> rcu_data") replaced DEFINE_PER_CPU by DEFINE_PER_CPU_SHARED_ALIGNED in
>>> the definition of rcu_sched and rcu_bh without updating
>>> kernel/rcu/tree.h.
>>>
>>> This makes clang report a section mismatch (-Wsection warning) when
>>> building LLVMLinux because the variables are declared in .data..percpu
>>> but defined in .data..percpu..shared_aligned.
>>>
>>> Signed-off-by: Nicolas Iooss
>>
>> Good catch.
>> Reviewed-by: Josh Triplett
>
> Agreed, good catch! But don't we also need to worry about
> rcu_preempt_data?
Yes, I missed it because I didn't know that allyesconfig/allmodconfig
does not select CONFIG_PREEMPT (it selects CONFIG_PREEMPT_NONE instead).
> Also, given that tree_trace.c now uses iterators
> rather than direct access via the per-CPU variables, wouldn't the
> following be more appropriate? (-Very- lightly tested.)
This doesn't work with CONFIG_TREE_RCU_TRACE, because
kernel/rcu/tree_trace.c uses rcu_preempt_state (in v4.1-rc2). I've
successfully built an allmodconfig+CONFIG_PREEMPT kernel for x86_64 with
the following patch (I have not tested the result).
Anyway, thanks for your quick replies.
Nicolas
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -91,7 +91,7 @@ static const char *tp_##sname##_varname __used
__tracepoint_string = sname##_var
#define RCU_STATE_INITIALIZER(sname, sabbr, cr) \
DEFINE_RCU_TPS(sname) \
-DEFINE_PER_CPU_SHARED_ALIGNED(struct rcu_data, sname##_data); \
+static DEFINE_PER_CPU_SHARED_ALIGNED(struct rcu_data, sname##_data); \
struct rcu_state sname##_state = { \
.level = { &sname##_state.node[0] }, \
.rda = &sname##_data, \
--- a/kernel/rcu/tree.h
+++ b/kernel/rcu/tree.h
@@ -519,14 +519,11 @@ extern struct list_head rcu_struct_flavors;
* RCU implementation internal declarations:
*/
extern struct rcu_state rcu_sched_state;
-DECLARE_PER_CPU(struct rcu_data, rcu_sched_data);
extern struct rcu_state rcu_bh_state;
-DECLARE_PER_CPU(struct rcu_data, rcu_bh_data);
#ifdef CONFIG_PREEMPT_RCU
extern struct rcu_state rcu_preempt_state;
-DECLARE_PER_CPU(struct rcu_data, rcu_preempt_data);
#endif /* #ifdef CONFIG_PREEMPT_RCU */
#ifdef CONFIG_RCU_BOOST
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] rcu: make rcu_*_data variables static
rcu_bh_data, rcu_sched_data and rcu_preempt_data are never used outside
kernel/rcu/tree.c and thus can be made static.
Doing so fixes a section mismatch warning reported by clang when
building LLVMLinux with -Wsection, because these variables were declared
in .data..percpu and defined in .data..percpu..shared_aligned since
commit 11bbb235c26f ("rcu: Use DEFINE_PER_CPU_SHARED_ALIGNED for
rcu_data").
Signed-off-by: Nicolas Iooss
---
kernel/rcu/tree.c | 2 +-
kernel/rcu/tree.h | 3 ---
2 files changed, 1 insertion(+), 4 deletions(-)
diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 233165da782f..e4a607fc5ad0 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -91,7 +91,7 @@ static const char *tp_##sname##_varname __used
__tracepoint_string = sname##_var
#define RCU_STATE_INITIALIZER(sname, sabbr, cr) \
DEFINE_RCU_TPS(sname) \
-DEFINE_PER_CPU_SHARED_ALIGNED(struct rcu_data, sname##_data); \
+static DEFINE_PER_CPU_SHARED_ALIGNED(struct rcu_data, sname##_data); \
struct rcu_state sname##_state = { \
.level = { &sname##_state.node[0] }, \
.rda = &sname##_data, \
diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
index a69d3dab2ec4..0c32f730d033 100644
--- a/kernel/rcu/tree.h
+++ b/kernel/rcu/tree.h
@@ -519,14 +519,11 @@ extern struct list_head rcu_struct_flavors;
* RCU implementation internal declarations:
*/
extern struct rcu_state rcu_sched_state;
-DECLARE_PER_CPU(struct rcu_data, rcu_sched_data);
extern struct rcu_state rcu_bh_state;
-DECLARE_PER_CPU(struct rcu_data, rcu_bh_data);
#ifdef CONFIG_PREEMPT_RCU
extern struct rcu_state rcu_preempt_state;
-DECLARE_PER_CPU(struct rcu_data, rcu_preempt_data);
#endif /* #ifdef CONFIG_PREEMPT_RCU */
#ifdef CONFIG_RCU_BOOST
--
2.3.6
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] coredump: use from_kuid/kgid_munged when formatting corename
On 05/06/2015 03:13 AM, Eric W. Biederman wrote: > Nicolas Iooss writes: > >> When adding __printf attribute to cn_printf, gcc reports some issues: >> >> fs/coredump.c:213:5: warning: format '%d' expects argument of type >> 'int', but argument 3 has type 'kuid_t' [-Wformat=] >>err = cn_printf(cn, "%d", cred->uid); >>^ >> fs/coredump.c:217:5: warning: format '%d' expects argument of type >> 'int', but argument 3 has type 'kgid_t' [-Wformat=] >>err = cn_printf(cn, "%d", cred->gid); >>^ >> >> These warnings come from the fact that the value of uid/gid needs to be >> extracted from the kuid_t/kgid_t structure before being used as an >> integer. More precisely, cred->uid and cred->gid need to be converted >> to either user-namespace uid/gid or to init_user_ns uid/gid. >> >> As Documentation/sysctl/kernel.txt does not specify which user namespace >> is used to translate %u and %g in core_pattern, but lowercase %p and %i >> are used to format pid/tid in the current process namespace, it seems >> intuitive that lowercase %u and %g use the current user namespace. > > I love the logic of lower vs upper case letters in the selection of how > an identifier should be used. Unfortunately I can't support it. > > Converting to anything other than init_user_ns will actually be an ABI > break. Which in practice should trump everything else. I agree. Initially I thought that my patch introduced only a minor change to make the ABI more consistent, but if you think this change is actually large enough to be considered a "not-wanted ABI break", I will follow your decision. > Further only the global root user can set this value, which largely > implies that the program setting the core dump patter will expect the > values to be in the initial user namespace. Ok, I missed this. The main source of my confusion is that the coredump uses the current mount namespace to create files (I tested using a Docker container with core_pattern set to something in /tmp) and the global process/mount namespaces when using pipes (tested with systemd-coredump). > In practice your patch allows any user to put any uid they want on core > files which seems to make the uid parameter useless. > > So for all of the reasons above I think we need to print uids and gids > in the initial user namespace unless explicitly requested to do so. So be it. I'll update my patches and send them again. In order to make things clearer, I also would like to modify the documentation of %u and %g, with something like: --- a/Documentation/sysctl/kernel.txt +++ b/Documentation/sysctl/kernel.txt @@ -197,8 +197,8 @@ %P global pid (init PID namespace) %i tid %I global tid (init PID namespace) - %u uid - %g gid + %u uid (in init user namespace) + %g gid (in init user namespace) %d dump mode, matches PR_SET_DUMPABLE and /proc/sys/fs/suid_dumpable %s signal number Would such a change be accepted, and if yes is it better to put it in its own commit or in the same as the one modifying the code? Thanks, Nicolas -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] rcu: declare rcu_data variables in the section they are defined in
Commit 11bbb235c26f ("rcu: Use DEFINE_PER_CPU_SHARED_ALIGNED for
rcu_data") replaced DEFINE_PER_CPU by DEFINE_PER_CPU_SHARED_ALIGNED in
the definition of rcu_sched and rcu_bh without updating
kernel/rcu/tree.h.
This makes clang report a section mismatch (-Wsection warning) when
building LLVMLinux because the variables are declared in .data..percpu
but defined in .data..percpu..shared_aligned.
Signed-off-by: Nicolas Iooss
---
kernel/rcu/tree.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
index a69d3dab2ec4..c5e85b27a79f 100644
--- a/kernel/rcu/tree.h
+++ b/kernel/rcu/tree.h
@@ -519,10 +519,10 @@ extern struct list_head rcu_struct_flavors;
* RCU implementation internal declarations:
*/
extern struct rcu_state rcu_sched_state;
-DECLARE_PER_CPU(struct rcu_data, rcu_sched_data);
+DECLARE_PER_CPU_SHARED_ALIGNED(struct rcu_data, rcu_sched_data);
extern struct rcu_state rcu_bh_state;
-DECLARE_PER_CPU(struct rcu_data, rcu_bh_data);
+DECLARE_PER_CPU_SHARED_ALIGNED(struct rcu_data, rcu_bh_data);
#ifdef CONFIG_PREEMPT_RCU
extern struct rcu_state rcu_preempt_state;
--
2.3.6
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] tracing: remove ftrace_output_event prototype
The prototype of ftrace_output_event was added by commit 1d6bae966e90
("tracing: Move raw output code from macro to standalone function")
but this function was not defined anywhere, and is still nowhere to be
found.
Signed-off-by: Nicolas Iooss
---
When sending the patch, scripts/get_maintainer.pl did not found any
match in MAINTAINERS. Would a patch which adds
"F: include/linux/ftrace*.h" to TRACING entry be accepted?
include/linux/ftrace_event.h | 3 ---
1 file changed, 3 deletions(-)
diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
index 46e83c2156c6..6382de706362 100644
--- a/include/linux/ftrace_event.h
+++ b/include/linux/ftrace_event.h
@@ -219,9 +219,6 @@ struct ftrace_event_class {
extern int ftrace_event_reg(struct ftrace_event_call *event,
enum trace_reg type, void *data);
-int ftrace_output_event(struct trace_iterator *iter, struct ftrace_event_call
*event,
- char *fmt, ...);
-
int ftrace_event_define_field(struct ftrace_event_call *call,
char *type, int len, char *item, int offset,
int field_size, int sign, int filter);
--
2.3.6
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/2] coredump: use from_kuid/kgid_munged when formatting corename
When adding __printf attribute to cn_printf, gcc reports some issues: fs/coredump.c:213:5: warning: format '%d' expects argument of type 'int', but argument 3 has type 'kuid_t' [-Wformat=] err = cn_printf(cn, "%d", cred->uid); ^ fs/coredump.c:217:5: warning: format '%d' expects argument of type 'int', but argument 3 has type 'kgid_t' [-Wformat=] err = cn_printf(cn, "%d", cred->gid); ^ These warnings come from the fact that the value of uid/gid needs to be extracted from the kuid_t/kgid_t structure before being used as an integer. More precisely, cred->uid and cred->gid need to be converted to either user-namespace uid/gid or to init_user_ns uid/gid. As Documentation/sysctl/kernel.txt does not specify which user namespace is used to translate %u and %g in core_pattern, but lowercase %p and %i are used to format pid/tid in the current process namespace, it seems intuitive that lowercase %u and %g use the current user namespace. While at it, format uid and gid values with %u instead of %d because uid_t/__kernel_uid32_t and gid_t/__kernel_gid32_t are unsigned int. Signed-off-by: Nicolas Iooss --- fs/coredump.c | 8 ++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/fs/coredump.c b/fs/coredump.c index bbbe139ab280..99c8af640c5a 100644 --- a/fs/coredump.c +++ b/fs/coredump.c @@ -209,11 +209,15 @@ static int format_corename(struct core_name *cn, struct coredump_params *cprm) break; /* uid */ case 'u': - err = cn_printf(cn, "%d", cred->uid); + err = cn_printf(cn, "%u", + from_kuid_munged(cred->user_ns, +cred->uid)); break; /* gid */ case 'g': - err = cn_printf(cn, "%d", cred->gid); + err = cn_printf(cn, "%u", + from_kgid_munged(cred->user_ns, +cred->gid)); break; case 'd': err = cn_printf(cn, "%d", -- 2.3.6 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/2] coredump: add __printf attribute to cn_*printf functions
This allows detecting improper format string at build time, like:
fs/coredump.c:225:5: warning: format '%ld' expects argument of type
'long int', but argument 3 has type 'int' [-Wformat=]
err = cn_printf(cn, "%ld", cprm->siginfo->si_signo);
^
As si_signo is always an int, the format should be %d here.
Signed-off-by: Nicolas Iooss
---
fs/coredump.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/fs/coredump.c b/fs/coredump.c
index 99c8af640c5a..30fc1a83cb09 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -70,7 +70,8 @@ static int expand_corename(struct core_name *cn, int size)
return 0;
}
-static int cn_vprintf(struct core_name *cn, const char *fmt, va_list arg)
+static __printf(2, 0) int cn_vprintf(struct core_name *cn, const char *fmt,
+va_list arg)
{
int free, need;
va_list arg_copy;
@@ -93,7 +94,7 @@ again:
return -ENOMEM;
}
-static int cn_printf(struct core_name *cn, const char *fmt, ...)
+static __printf(2, 3) int cn_printf(struct core_name *cn, const char *fmt, ...)
{
va_list arg;
int ret;
@@ -105,7 +106,8 @@ static int cn_printf(struct core_name *cn, const char *fmt,
...)
return ret;
}
-static int cn_esc_printf(struct core_name *cn, const char *fmt, ...)
+static __printf(2, 3)
+int cn_esc_printf(struct core_name *cn, const char *fmt, ...)
{
int cur = cn->used;
va_list arg;
@@ -225,7 +227,7 @@ static int format_corename(struct core_name *cn, struct
coredump_params *cprm)
break;
/* signal that caused the coredump */
case 's':
- err = cn_printf(cn, "%ld",
cprm->siginfo->si_signo);
+ err = cn_printf(cn, "%d",
cprm->siginfo->si_signo);
break;
/* UNIX time of coredump */
case 't': {
--
2.3.6
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
wl18xx: Bad format for rx_frames_per_rates in debugfs?
Hello,
While adding __printf attributes to several functions in the kernel, I
got a surprising gcc warning in drivers/net/wireless/ti/wl18xx/debugfs.c
about "format '%u' expects argument of type 'unsigned int', but argument
5 has type 'u32 *'".
Indeed it seems that commit c5d94169e818 ("wl18xx: use new fw stats
structures") [1] introduced an array field "u32 rx_frames_per_rates[50]"
in struct wl18xx_acx_rx_rate_stat but is using
WL18XX_DEBUGFS_FWSTATS_FILE(rx_rate, rx_frames_per_rates, "%u"); instead
of something like WL18XX_DEBUGFS_FWSTATS_FILE_ARRAY(rx_rate,
rx_frames_per_rates, 50); for displaying this value. So I believe that
currently the rx_rate entry in debugfs contains a kernel pointer instead
of the actual data. As I don't have the hardware to test I can't be
sure of it.
Is this a real bug which needs to be fixed or something weird I haven't
understood yet?
Thanks
--
Nicolas
[1]
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=c5d94169e8189d02dfbd6143411908357865d777
PS: I got this gcc warning by adding __printf(4, 5) to
wl1271_format_buffer() prototype in
drivers/net/wireless/ti/wlcore/debugfs.h:
In file included from
/usr/src/linux/drivers/net/wireless/ti/wl18xx/debugfs.c:23:0:
/usr/src/linux/drivers/net/wireless/ti/wl18xx/debugfs.c: In function
'rx_rate_rx_frames_per_rates_read':
/usr/src/linux/drivers/net/wireless/ti/wl18xx/debugfs.c:34:32:
error: format '%u' expects argument of type 'unsigned int', but argument
5 has type 'u32 *' [-Werror=format=]
DEBUGFS_FWSTATS_FILE(a, b, c, wl18xx_acx_statistics)
^
/usr/src/linux/drivers/net/wireless/ti/wl18xx/../wlcore/debugfs.h:77:9:
note: in definition of macro 'DEBUGFS_FWSTATS_FILE'
struct struct_type *stats = wl->stats.fw_stats; \
^
/usr/src/linux/drivers/net/wireless/ti/wl18xx/debugfs.c:142:1: note:
in expansion of macro 'WL18XX_DEBUGFS_FWSTATS_FILE'
WL18XX_DEBUGFS_FWSTATS_FILE(rx_rate, rx_frames_per_rates, "%u");
^
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] wl18xx: show rx_frames_per_rates as an array as it really is
In struct wl18xx_acx_rx_rate_stat, rx_frames_per_rates field is an
array, not a number. This means WL18XX_DEBUGFS_FWSTATS_FILE can't be
used to display this field in debugfs (it would display a pointer, not
the actual data). Use WL18XX_DEBUGFS_FWSTATS_FILE_ARRAY instead.
This bug has been found by adding a __printf attribute to
wl1271_format_buffer. gcc complained about "format '%u' expects
argument of type 'unsigned int', but argument 5 has type 'u32 *'".
Fixes: c5d94169e818 ("wl18xx: use new fw stats structures")
Signed-off-by: Nicolas Iooss
---
This patch is only compile-tested as I haven't got the hardware to
test it live.
drivers/net/wireless/ti/wl18xx/debugfs.c | 2 +-
drivers/net/wireless/ti/wlcore/debugfs.h | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/ti/wl18xx/debugfs.c
b/drivers/net/wireless/ti/wl18xx/debugfs.c
index c93fae95baac..5fbd2230f372 100644
--- a/drivers/net/wireless/ti/wl18xx/debugfs.c
+++ b/drivers/net/wireless/ti/wl18xx/debugfs.c
@@ -139,7 +139,7 @@ WL18XX_DEBUGFS_FWSTATS_FILE(rx_filter, protection_filter,
"%u");
WL18XX_DEBUGFS_FWSTATS_FILE(rx_filter, accum_arp_pend_requests, "%u");
WL18XX_DEBUGFS_FWSTATS_FILE(rx_filter, max_arp_queue_dep, "%u");
-WL18XX_DEBUGFS_FWSTATS_FILE(rx_rate, rx_frames_per_rates, "%u");
+WL18XX_DEBUGFS_FWSTATS_FILE_ARRAY(rx_rate, rx_frames_per_rates, 50);
WL18XX_DEBUGFS_FWSTATS_FILE_ARRAY(aggr_size, tx_agg_vs_rate,
AGGR_STATS_TX_AGG*AGGR_STATS_TX_RATE);
diff --git a/drivers/net/wireless/ti/wlcore/debugfs.h
b/drivers/net/wireless/ti/wlcore/debugfs.h
index 0f2cfb0d2a9e..bf14676e6515 100644
--- a/drivers/net/wireless/ti/wlcore/debugfs.h
+++ b/drivers/net/wireless/ti/wlcore/debugfs.h
@@ -26,8 +26,8 @@
#include "wlcore.h"
-int wl1271_format_buffer(char __user *userbuf, size_t count,
-loff_t *ppos, char *fmt, ...);
+__printf(4, 5) int wl1271_format_buffer(char __user *userbuf, size_t count,
+ loff_t *ppos, char *fmt, ...);
int wl1271_debugfs_init(struct wl1271 *wl);
void wl1271_debugfs_exit(struct wl1271 *wl);
--
2.3.2
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
nfs: wrong function to increment NFSIOS_READPAGES/WRITEPAGES stat counters?
Hello,
Since commit 5a254d08b086 ("nfs: replace nfs_add_stats with
nfs_inc_stats when add one") [1], nfs_readpage and nfs_do_writepage use
nfs_inc_stats to increment NFSIOS_READPAGES and NFSIOS_WRITEPAGES
instead of nfs_add_stats.
However nfs_inc_stats is not similar as nfs_add_stats with parameter 1
because these functions work on distinct stats:
nfs_inc_stats increments stats from "enum nfs_stat_eventcounters" (in
server->io_stats->events) and nfs_add_stats those from "enum
nfs_stat_bytecounters" (in server->io_stats->bytes), according to their
implementations in fs/nfs/iostat.h [2].
If I understand the code correctly, "nfs_inc_stats(inode,
NFSIOS_READPAGES);" is in fact executed as "nfs_inc_stats(inode,
NFSIOS_VFSACCESS);" and "nfs_inc_stats(inode, NFSIOS_WRITEPAGES);" as
"nfs_inc_stats(inode, NFSIOS_VFSUPDATEPAGE);".
As this looks like a bug to me, I am reporting it in hope it could be
fixed, for example by reverting 5a254d08b086. Of course, I may be
wrong, in which case an explanation about what I missed in my analysis
would be appreciated.
By the way, I found this while trying to compile LLVMLinux on current
master branch of Linux. clang reported the following warnings:
fs/nfs/read.c:287:23: warning: implicit conversion from enumeration type
'enum nfs_stat_bytecounters' to different enumeration type 'enum
nfs_stat_eventcounters' [-Wenum-conversion]
nfs_inc_stats(inode, NFSIOS_READPAGES);
~^~~~
fs/nfs/write.c:583:23: warning: implicit conversion from enumeration
type 'enum nfs_stat_bytecounters' to different enumeration type 'enum
nfs_stat_eventcounters' [-Wenum-conversion]
nfs_inc_stats(inode, NFSIOS_WRITEPAGES);
~^
Thanks,
Nicolas
[1]
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=5a254d08b086d80cbead2ebcee6d2a4b3a15587a
[2]
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/fs/nfs/iostat.h?id=90a5a895cc8b284ac522757a01de15e36710c2b9
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] reiserfs: fix several reiserfs_warning calls
Since commit 45b03d5e8e67 ("reiserfs: rework reiserfs_warning"),
reiserfs_warning takes an id and a format as arguments, not a single
format argument. However 4 calls still follow the old interface.
Update them.
This bug was initially found by adding __printf(4, 5) attribute to
__reiserfs_warning and using "gcc -Wformat=2" when building the
kernel.
Fixes: 45b03d5e8e67 ("reiserfs: rework reiserfs_warning")
Signed-off-by: Nicolas Iooss
---
fs/reiserfs/bitmap.c | 4 ++--
fs/reiserfs/journal.c | 4 ++--
fs/reiserfs/procfs.c | 4 ++--
3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/fs/reiserfs/bitmap.c b/fs/reiserfs/bitmap.c
index dc198bc64c61..1d02f184863d 100644
--- a/fs/reiserfs/bitmap.c
+++ b/fs/reiserfs/bitmap.c
@@ -1423,8 +1423,8 @@ struct buffer_head *reiserfs_read_bitmap_block(struct
super_block *sb,
bh = sb_bread(sb, block);
if (bh == NULL)
- reiserfs_warning(sb, "sh-2029: %s: bitmap block (#%u) "
-"reading failed", __func__, block);
+ reiserfs_warning(sb, "sh-2029",
+"bitmap block (#%u) reading failed", block);
else {
if (buffer_locked(bh)) {
int depth;
diff --git a/fs/reiserfs/journal.c b/fs/reiserfs/journal.c
index 9d6486d416a3..bb08e81ef9ec 100644
--- a/fs/reiserfs/journal.c
+++ b/fs/reiserfs/journal.c
@@ -2643,8 +2643,8 @@ static int journal_init_dev(struct super_block *super,
if (IS_ERR(journal->j_dev_bd)) {
result = PTR_ERR(journal->j_dev_bd);
journal->j_dev_bd = NULL;
- reiserfs_warning(super,
-"journal_init_dev: Cannot open '%s': %i",
+ reiserfs_warning(super, NULL,
+"Cannot open '%s': %i",
jdev_name, result);
return result;
}
diff --git a/fs/reiserfs/procfs.c b/fs/reiserfs/procfs.c
index 621b9f381fe1..e6a1b2c34ef6 100644
--- a/fs/reiserfs/procfs.c
+++ b/fs/reiserfs/procfs.c
@@ -436,7 +436,7 @@ int reiserfs_proc_info_init(struct super_block *sb)
add_file(sb, "journal", show_journal);
return 0;
}
- reiserfs_warning(sb, "cannot create /proc/%s/%s",
+ reiserfs_warning(sb, NULL, "cannot create /proc/%s/%s",
proc_info_root_name, b);
return 1;
}
@@ -465,7 +465,7 @@ int reiserfs_proc_info_global_init(void)
if (proc_info_root == NULL) {
proc_info_root = proc_mkdir(proc_info_root_name, NULL);
if (!proc_info_root) {
- reiserfs_warning(NULL, "cannot create /proc/%s",
+ reiserfs_warning(NULL, NULL, "cannot create /proc/%s",
proc_info_root_name);
return 1;
}
--
2.3.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] coredump: fix cn_printf formatting warnings
Add __printf attributes to cn_*printf functions. With these, gcc says:
fs/coredump.c:213:5: warning: format '%d' expects argument of type
'int', but argument 3 has type 'kuid_t' [-Wformat=]
err = cn_printf(cn, "%d", cred->uid);
^
fs/coredump.c:217:5: warning: format '%d' expects argument of type
'int', but argument 3 has type 'kgid_t' [-Wformat=]
err = cn_printf(cn, "%d", cred->gid);
^
fs/coredump.c:225:5: warning: format '%ld' expects argument of type
'long int', but argument 3 has type 'int' [-Wformat=]
err = cn_printf(cn, "%ld", cprm->siginfo->si_signo);
^
The third warning is easily fixed as si_signo is always an int.
For the two others, cred->uid and cred->gid need to be converted to
either a user-namespace UID/GID or to init_user_ns UID/GID. As
Documentation/sysctl/kernel.txt does not specify which user namespace is
used to translate %u and %g in core_pattern, but lowercase %p and %i are
used to format pid/tid in current process namespace, it seems intuitive
that lowercase %u and %g use the current user namespace. So implement
this.
Signed-off-by: Nicolas Iooss
---
fs/coredump.c | 18 --
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/fs/coredump.c b/fs/coredump.c
index f319926ddf8c..762d827825a8 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -70,7 +70,8 @@ static int expand_corename(struct core_name *cn, int size)
return 0;
}
-static int cn_vprintf(struct core_name *cn, const char *fmt, va_list arg)
+static __printf(2, 0) int cn_vprintf(struct core_name *cn, const char *fmt,
+va_list arg)
{
int free, need;
va_list arg_copy;
@@ -93,7 +94,7 @@ again:
return -ENOMEM;
}
-static int cn_printf(struct core_name *cn, const char *fmt, ...)
+static __printf(2, 3) int cn_printf(struct core_name *cn, const char *fmt, ...)
{
va_list arg;
int ret;
@@ -105,7 +106,8 @@ static int cn_printf(struct core_name *cn, const char *fmt,
...)
return ret;
}
-static int cn_esc_printf(struct core_name *cn, const char *fmt, ...)
+static __printf(2, 3)
+int cn_esc_printf(struct core_name *cn, const char *fmt, ...)
{
int cur = cn->used;
va_list arg;
@@ -209,11 +211,15 @@ static int format_corename(struct core_name *cn, struct
coredump_params *cprm)
break;
/* uid */
case 'u':
- err = cn_printf(cn, "%d", cred->uid);
+ err = cn_printf(cn, "%d",
+ from_kuid_munged(cred->user_ns,
+cred->uid));
break;
/* gid */
case 'g':
- err = cn_printf(cn, "%d", cred->gid);
+ err = cn_printf(cn, "%d",
+ from_kgid_munged(cred->user_ns,
+cred->gid));
break;
case 'd':
err = cn_printf(cn, "%d",
@@ -221,7 +227,7 @@ static int format_corename(struct core_name *cn, struct
coredump_params *cprm)
break;
/* signal that caused the coredump */
case 's':
- err = cn_printf(cn, "%ld",
cprm->siginfo->si_signo);
+ err = cn_printf(cn, "%d",
cprm->siginfo->si_signo);
break;
/* UNIX time of coredump */
case 't': {
--
2.3.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] kdbus: fix minor typo in the walk-through example
s/receveiver/receiver/ Signed-off-by: Nicolas Iooss --- samples/kdbus/kdbus-workers.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/samples/kdbus/kdbus-workers.c b/samples/kdbus/kdbus-workers.c index d1d8f7a7697b..d331e0186899 100644 --- a/samples/kdbus/kdbus-workers.c +++ b/samples/kdbus/kdbus-workers.c @@ -787,8 +787,8 @@ static int child_run(struct child *c) * The 2nd item contains a vector to memory we want to send. It * can be content of any type. In our case, we're sending a one-byte * string only. The memory referenced by this item will be copied into -* the pool of the receveiver connection, and does not need to be -* valid after the command is employed. +* the pool of the receiver connection, and does not need to be valid +* after the command is employed. */ item = KDBUS_ITEM_NEXT(item); item->type = KDBUS_ITEM_PAYLOAD_VEC; -- 2.3.2 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] coredump: fix cn_printf formatting warnings
Ping? Comments (or NACK or ACK) would be greatly appreciated on this
patch. Of course, if you prefer to use init_user_ns instead of
cred->user_ns, I can send an other patch.
Nicolas
On 03/06/2015 09:00 PM, Nicolas Iooss wrote:
> Add __printf attributes to cn_*printf functions. With these, gcc says:
>
> fs/coredump.c:213:5: warning: format '%d' expects argument of type
> 'int', but argument 3 has type 'kuid_t' [-Wformat=]
>err = cn_printf(cn, "%d", cred->uid);
>^
> fs/coredump.c:217:5: warning: format '%d' expects argument of type
> 'int', but argument 3 has type 'kgid_t' [-Wformat=]
>err = cn_printf(cn, "%d", cred->gid);
>^
> fs/coredump.c:225:5: warning: format '%ld' expects argument of type
> 'long int', but argument 3 has type 'int' [-Wformat=]
>err = cn_printf(cn, "%ld", cprm->siginfo->si_signo);
>^
>
> The third warning is easily fixed as si_signo is always an int.
>
> For the two others, cred->uid and cred->gid need to be converted to
> either a user-namespace UID/GID or to init_user_ns UID/GID. As
> Documentation/sysctl/kernel.txt does not specify which user namespace is
> used to translate %u and %g in core_pattern, but lowercase %p and %i are
> used to format pid/tid in current process namespace, it seems intuitive
> that lowercase %u and %g use the current user namespace. So implement
> this.
>
> Signed-off-by: Nicolas Iooss
> ---
> fs/coredump.c | 18 --
> 1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/fs/coredump.c b/fs/coredump.c
> index f319926ddf8c..762d827825a8 100644
> --- a/fs/coredump.c
> +++ b/fs/coredump.c
> @@ -70,7 +70,8 @@ static int expand_corename(struct core_name *cn, int size)
> return 0;
> }
>
> -static int cn_vprintf(struct core_name *cn, const char *fmt, va_list arg)
> +static __printf(2, 0) int cn_vprintf(struct core_name *cn, const char *fmt,
> + va_list arg)
> {
> int free, need;
> va_list arg_copy;
> @@ -93,7 +94,7 @@ again:
> return -ENOMEM;
> }
>
> -static int cn_printf(struct core_name *cn, const char *fmt, ...)
> +static __printf(2, 3) int cn_printf(struct core_name *cn, const char *fmt,
> ...)
> {
> va_list arg;
> int ret;
> @@ -105,7 +106,8 @@ static int cn_printf(struct core_name *cn, const char
> *fmt, ...)
> return ret;
> }
>
> -static int cn_esc_printf(struct core_name *cn, const char *fmt, ...)
> +static __printf(2, 3)
> +int cn_esc_printf(struct core_name *cn, const char *fmt, ...)
> {
> int cur = cn->used;
> va_list arg;
> @@ -209,11 +211,15 @@ static int format_corename(struct core_name *cn, struct
> coredump_params *cprm)
> break;
> /* uid */
> case 'u':
> - err = cn_printf(cn, "%d", cred->uid);
> + err = cn_printf(cn, "%d",
> + from_kuid_munged(cred->user_ns,
> + cred->uid));
> break;
> /* gid */
> case 'g':
> - err = cn_printf(cn, "%d", cred->gid);
> + err = cn_printf(cn, "%d",
> + from_kgid_munged(cred->user_ns,
> + cred->gid));
> break;
> case 'd':
> err = cn_printf(cn, "%d",
> @@ -221,7 +227,7 @@ static int format_corename(struct core_name *cn, struct
> coredump_params *cprm)
> break;
> /* signal that caused the coredump */
> case 's':
> - err = cn_printf(cn, "%ld",
> cprm->siginfo->si_signo);
> + err = cn_printf(cn, "%d",
> cprm->siginfo->si_signo);
> break;
> /* UNIX time of coredump */
> case 't': {
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] NFC: st21nfca: fix st21nfca_get_iso14443_3_uid data copy
Hello, I have not received any comments about this patch so I'm sending it again. Could you please review it? Nicolas On 03/03/2015 12:58 PM, Nicolas Iooss wrote: > st21nfca_get_iso14443_3_uid() does not correctly copy the uid from > uid_skb->data to its gate parameter. "gate = uid_skb->data;" only puts > a pointer to uid_skb->data to the local variable gate. This means that > in st21nfca_hci_target_from_gate() the content of "u8 > uid[NFC_NFCID1_MAXSIZE]" local variable is never initialized before > being used in memcpy(target->nfcid1, uid, len). > > Fix this by replacing the local variable assignment with a memcpy. > > This was found by compiling Linux with "gcc -Wunused-but-set-parameter". > > Signed-off-by: Nicolas Iooss > --- > > As I did not get any reply from https://lkml.org/lkml/2015/2/28/25 and > got confirmation by other people that this may be a real bug, I am now > sending a patch to fix it. > > drivers/nfc/st21nfca/st21nfca.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/nfc/st21nfca/st21nfca.c b/drivers/nfc/st21nfca/st21nfca.c > index 24d3d240d5f4..ff70d2838b29 100644 > --- a/drivers/nfc/st21nfca/st21nfca.c > +++ b/drivers/nfc/st21nfca/st21nfca.c > @@ -588,7 +588,7 @@ static int st21nfca_get_iso14443_3_uid(struct nfc_hci_dev > *hdev, u8 *gate, > goto exit; > } > > - gate = uid_skb->data; > + memcpy(gate, uid_skb->data, uid_skb->len); > *len = uid_skb->len; > exit: > kfree_skb(uid_skb); > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
reiserfs: inconsistent format in __RASSERT
Hello,
When adding a __printf attribute to reiserfs_panic, gcc reported an
inconsistent format for __RASSERT. This macro is currently defined in
fs/reiserfs/reiserfs.h as:
reiserfs_panic(NULL, "assertion failure", "(" #cond ") at " \
__FILE__ ":%i:%s: " format "\n",\
in_interrupt() ? -1 : task_pid_nr(current), \
__LINE__, __func__ , ##args);
In the format string, the first parameter is a line number, but in the
arguments there is a PID before. Before c3a9c2109f84 ("reiserfs: rework
reiserfs_panic") [1], the format string began with "reiserfs[%i]" [2],
which explains the PID in the arguments.
I see three possibilities:
* I missed something in my analysis and in fact the PID argument is
processed by reiserfs_panic (don't know where), or
* the PID argument is not used and should be removed, or
* the PID is useful and "[%i]" should be added somewhere in the format
string.
Which one would you prefer?
Also, I found this when building the kernel with "allmodconfig" on
x86_64. With "defconfig" gcc does not report this error, but I guess it
is because without CONFIG_REISERFS_CHECK, __RASSERT is never used.
Regards,
Nicolas
[1]
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=c3a9c2109f84882b9b3178f6b1838d550d3df0ec
[2]
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/linux/reiserfs_fs.h?id=78b6513d2881f1a759fb9825a036d926392de084#n91
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: reiserfs: inconsistent format in __RASSERT
On 03/16/2015 09:05 PM, Jeff Mahoney wrote: > On 3/16/15 8:55 AM, Nicolas Iooss wrote: >> * I missed something in my analysis and in fact the PID argument >> is processed by reiserfs_panic (don't know where), or * the PID >> argument is not used and should be removed, or > > This, please. reiserfs_panic calls BUG(), which will contain the PID. Whoo, thanks for the quick answer. I will send a patch as soon as possible. >> * the PID is useful and "[%i]" should be added somewhere in the >> format string. > >> Which one would you prefer? > >> Also, I found this when building the kernel with "allmodconfig" on >> x86_64. With "defconfig" gcc does not report this error, but I >> guess it is because without CONFIG_REISERFS_CHECK, __RASSERT is >> never used. > > Yeah. If reiserfs was more actively maintained, what is currently > protected by CONFIG_REISERFS_CHECK would be handled a bit better. > There are ton of fsfuzzer bugs that would be caught by it and should > be handled using reiserfs_error. Unfortunately, it also enables some > heavy checks that make the file system very slow. > > Thanks for looking into this. It looks like it's been broken for a > while. I suppose the only saving grace is that it would crash in a > path that crashes on purpose a few lines later. Yes, and this is also why I believe this bug is not a security issue nor something which needs an urgent fix. Thanks, Nicolas -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] reiserfs: fix __RASSERT format string
__RASSERT format string does not use the PID argument. reiserfs_panic
arguments are therefore formatted with the wrong format specifier (for
example __LINE__ with %s). This bug was introduced when commit
c3a9c2109f84 ("reiserfs: rework reiserfs_panic") removed a
"reiserfs[%i]" prefix.
This bug is only triggered when using CONFIG_REISERFS_CHECK, otherwise
__RASSERT is never used.
Signed-off-by: Nicolas Iooss
Fixes: c3a9c2109f84 ("reiserfs: rework reiserfs_panic")
---
fs/reiserfs/reiserfs.h | 1 -
1 file changed, 1 deletion(-)
diff --git a/fs/reiserfs/reiserfs.h b/fs/reiserfs/reiserfs.h
index bb79cddf0a1f..2adcde137c3f 100644
--- a/fs/reiserfs/reiserfs.h
+++ b/fs/reiserfs/reiserfs.h
@@ -910,7 +910,6 @@ do {
\
if (!(cond))\
reiserfs_panic(NULL, "assertion failure", "(" #cond ") at " \
__FILE__ ":%i:%s: " format "\n", \
- in_interrupt() ? -1 : task_pid_nr(current), \
__LINE__, __func__ , ##args);\
} while (0)
--
2.3.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] NFC: st21nfca: fix st21nfca_get_iso14443_3_uid data copy
st21nfca_get_iso14443_3_uid() does not correctly copy the uid from uid_skb->data to its gate parameter. "gate = uid_skb->data;" only puts a pointer to uid_skb->data to the local variable gate. This means that in st21nfca_hci_target_from_gate() the content of "u8 uid[NFC_NFCID1_MAXSIZE]" local variable is never initialized before being used in memcpy(target->nfcid1, uid, len). Fix this by replacing the local variable assignment with a memcpy. This was found by compiling Linux with "gcc -Wunused-but-set-parameter". Signed-off-by: Nicolas Iooss --- As I did not get any reply from https://lkml.org/lkml/2015/2/28/25 and got confirmation by other people that this may be a real bug, I am now sending a patch to fix it. drivers/nfc/st21nfca/st21nfca.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/nfc/st21nfca/st21nfca.c b/drivers/nfc/st21nfca/st21nfca.c index 24d3d240d5f4..ff70d2838b29 100644 --- a/drivers/nfc/st21nfca/st21nfca.c +++ b/drivers/nfc/st21nfca/st21nfca.c @@ -588,7 +588,7 @@ static int st21nfca_get_iso14443_3_uid(struct nfc_hci_dev *hdev, u8 *gate, goto exit; } - gate = uid_skb->data; + memcpy(gate, uid_skb->data, uid_skb->len); *len = uid_skb->len; exit: kfree_skb(uid_skb); -- 2.3.1 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] ihex: restore missing default in switch statement
Commit 2473238eac95 ("ihex: add support for CS:IP/EIP records") removes
the "default:" statement in the switch block, making the "return
usage();" line dead code and ihex2fw silently ignoring unknown options.
Restore this statement.
This bug was found by building with HOSTCC=clang and adding
-Wunreachable-code-return to HOSTCFLAGS.
Fixes: 2473238eac95 ("ihex: add support for CS:IP/EIP records")
Signed-off-by: Nicolas Iooss
---
firmware/ihex2fw.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/firmware/ihex2fw.c b/firmware/ihex2fw.c
index cf38e159131a..08d90e25abf0 100644
--- a/firmware/ihex2fw.c
+++ b/firmware/ihex2fw.c
@@ -86,6 +86,7 @@ int main(int argc, char **argv)
case 'j':
include_jump = 1;
break;
+ default:
return usage();
}
}
--
2.3.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] MAINTAINERS: fix UTF-8 encoding
Commit 41c9e95d641a ("MAINTAINERS: add Android driver entries")
introduced non-UTF8 characters in MAINTAINERS file. This breaks tools
like grep when using an UTF-8 locale:
$ grep -n drivers/android MAINTAINERS
Binary file MAINTAINERS matches
Replacing the characters by their UTF-8 counterparts fixes grep.
$ grep -n drivers/android MAINTAINERS
733:F: drivers/android/
This also fixes the display of
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/MAINTAINERS?id=v3.19#n713
Signed-off-by: Nicolas Iooss
---
MAINTAINERS | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index 42f686f2e4b2..39187c2848e5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -725,7 +725,7 @@ F: staging/iio/trigger/iio-trig-bfin-timer.c
ANDROID DRIVERS
M: Greg Kroah-Hartman
-M: Arve Hj?nnev?g
+M: Arve Hj??nnev??g
M: Riley Andrews
T: git git://git.kernel.org/pub/scm/linux/kernel/gregkh/staging.git
L: de...@driverdev.osuosl.org
--
2.3.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] MAINTAINERS: fix UTF-8 encoding
On 03/04/2015 11:21 PM, Geert Uytterhoeven wrote: > On Wed, Mar 4, 2015 at 4:02 PM, Jiri Kosina wrote: >>> --- a/MAINTAINERS >>> +++ b/MAINTAINERS >>> @@ -725,7 +725,7 @@ F:staging/iio/trigger/iio-trig-bfin-timer.c >>> >>> ANDROID DRIVERS >>> M: Greg Kroah-Hartman >>> -M: Arve Hj?nnev?g >>> +M: Arve Hj??nnev??g >> >> I don't really call that an improvement. >> >> The patch was sent as >> >> Content-Type: text/plain; charset=y >> >> 'charset=y' (a) doesn't make any sense whatsoever (b) directly contradicts >> the intent of the patch. > > Just another case of user answering all git questions with "y". > Been there, done that. Will be handled in git soon > http://www.spinics.net/lists/git/msg246024.html > Thanks for telling me what I did wrong. I confirm I did what you described, I still have the terminal open with the 2 questions git asked me: $ git send-email --to triv...@kernel.org --cc linux-kernel@vger.kernel.org 0001-MAINTAINERS-fix-UTF-8-encoding.patch 0001-MAINTAINERS-fix-UTF-8-encoding.patch The following files are 8bit, but do not declare a Content-Transfer-Encoding. 0001-MAINTAINERS-fix-UTF-8-encoding.patch Which 8bit encoding should I declare [UTF-8]? y (mbox) Adding cc: Nicolas Iooss from line 'From: Nicolas Iooss ' (body) Adding cc: Nicolas Iooss from line 'Signed-off-by: Nicolas Iooss ' From: Nicolas Iooss To: triv...@kernel.org Cc: linux-kernel@vger.kernel.org, Nicolas Iooss Subject: [PATCH] MAINTAINERS: fix UTF-8 encoding Date: Wed, 4 Mar 2015 22:38:47 +0800 Message-Id: <1425479927-6769-1-git-send-email-nicolas.iooss_li...@m4x.org> X-Mailer: git-send-email 2.3.1 MIME-Version: 1.0 Content-Type: text/plain; charset=y Content-Transfer-Encoding: 8bit Send this email? ([y]es|[n]o|[q]uit|[a]ll): y It is quite misleading to have both a "non-y" and a "yes/no" answer. Now that I know this, I will try not to do the same error the next times I send patches with both UTF-8 and iso-8859-15 characters. Should I send the patch again with proper encoding? Nicolas -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] MAINTAINERS: fix UTF-8 encoding
Commit 41c9e95d641a ("MAINTAINERS: add Android driver entries")
introduced non-UTF8 characters in MAINTAINERS file. This breaks tools
like grep when using an UTF-8 locale:
$ grep -n drivers/android MAINTAINERS
Binary file MAINTAINERS matches
Replacing the characters by their UTF-8 counterparts fixes grep.
$ grep -n drivers/android MAINTAINERS
733:F: drivers/android/
This also fixes the display of
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/MAINTAINERS?id=v3.19#n713
Signed-off-by: Nicolas Iooss
---
MAINTAINERS | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index 42f686f2e4b2..39187c2848e5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -725,7 +725,7 @@ F: staging/iio/trigger/iio-trig-bfin-timer.c
ANDROID DRIVERS
M: Greg Kroah-Hartman
-M: Arve Hj�nnev�g
+M: Arve Hjønnevåg
M: Riley Andrews
T: git git://git.kernel.org/pub/scm/linux/kernel/gregkh/staging.git
L: de...@driverdev.osuosl.org
--
2.3.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] proc: get a reference to the owning module when opening file
When a module creates a file in procfs and a program uses the file with
mmap, the .owner field of the file_operations structure is ignored.
This allows removing the module while the file is still being used.
Therefore this sequence of events leads to a kernel oops:
* load a module which creates a file in procfs with an mmap operation
* open the file
* use mmap on it
* rmmod the module
* call unmap
Here are parts of the oops caused by unmap:
[ 1234.337725] BUG: unable to handle kernel paging request at a030a268
[ 1234.338007] IP: [] remove_vma+0x24/0x70
[ 1234.338007] PGD 1c17067 PUD 1c18063 PMD 3d713067 PTE 0
[ 1234.338007] Oops: [#1] SMP
[ 1234.338007] Modules linked in: fuse rpcsec_gss_krb5 nfsv4
dns_resolver nfs fscache cfg80211 rfkill ppdev bochs_drm virtio_net
serio_raw parport_pc ttm pvpanic parport drm_kms_helper drm i2c_piix4
nfsd auth_rpcgss nfs_acl lockd sunrpc virtio_blk virtio_pci virtio_ring
virtio ata_generic pata_acpi [last unloaded: procfs_mmap]
[ 1234.338007] Call Trace:
[ 1234.338007] [] do_munmap+0x27f/0x3b0
[ 1234.338007] [] vm_munmap+0x41/0x60
[ 1234.338007] [] SyS_munmap+0x22/0x30
[ 1234.338007] [] system_call_fastpath+0x16/0x1b
Fix this by making proc_reg_open grab a reference to the module owning
pde->proc_fops.
More information and example code to reproduce this oops can be found on
https://bugzilla.kernel.org/show_bug.cgi?id=92511
Signed-off-by: Nicolas Iooss
---
fs/proc/inode.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/fs/proc/inode.c b/fs/proc/inode.c
index 8420a2f80811..5df17cb730fa 100644
--- a/fs/proc/inode.c
+++ b/fs/proc/inode.c
@@ -329,12 +329,16 @@ static int proc_reg_open(struct inode *inode, struct file
*file)
kfree(pdeo);
return -ENOENT;
}
+ fops_get(pde->proc_fops);
open = pde->proc_fops->open;
release = pde->proc_fops->release;
if (open)
rv = open(inode, file);
+ if (rv != 0)
+ fops_put(pde->proc_fops);
+
if (rv == 0 && release) {
/* To know what to release. */
pdeo->file = file;
@@ -361,6 +365,7 @@ static int proc_reg_release(struct inode *inode, struct
file *file)
}
}
spin_unlock(&pde->pde_unload_lock);
+ fops_put(pde->proc_fops);
return 0;
}
--
2.3.0
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] proc: get a reference to the owning module when opening file
On 02/18/2015 04:14 AM, Al Viro wrote: > On Wed, Feb 11, 2015 at 11:05:07AM +0800, Nicolas Iooss wrote: >> Fix this by making proc_reg_open grab a reference to the module owning >> pde->proc_fops. > > NAK. > > procfs doesn't pin modules while the file is opened, by design. And > it's not about to start. Background: > > * ->owner protects the wrong thing - it protects _code_ in methods, but > not the data structures. Preventing rmmod might act as a proxy in some > cases, but only when objects are _not_ dynamically created/removed. All right. I've found the relevant documentation for this [1]: what you say is already well documented and I was wrongly assuming things based on my experience with debugfs and character devices. This bad assumption came partly from the fact that do_dentry_open() takes a reference to inode->i_fop->owner [2], which is only released in __fput(). For character devices the file_operation structure is the one given to register_chrdev() (thanks to a call to replace_fops() [3]) and for debugfs files this structure is directly the one given to debugfs_create_file() [4]. As a consequence the semantics of ->owner is implicitly extended to protect the module which did register_chrdev() or debugfs_create_file() from being unloaded while the file is used. If this extension is a side-effect which must not be generalized, I agree with your point of view. > * what we want is to make sure that no new method calls might be issued > after procfs removal and that procfs removal will wait for method > calls already in progress to complete. Again, the only way it's relevant > to rmmod is that procfs removal is often triggered by module_exit. > BTW, debugfs is seriously broken in that respect - it does protect the > module, all right, but have an object removed earlier and you are fucked. Now I understand why commit 881adb853583 [5] was needed for procfs instead of using try_module_get()/module_put(). Thank you for having made this clear. For the debugfs part, I though that debugfs_remove() did something similar to unlink() for usual filesystems so that even if the file is used, the inode is only unlinked from debugfs and the file is not released. It is of course not safe to free data which could be used by the file without the file being released first. Nevertheless I considered that the scopes were made so that, while the file was being used, the module which has registered the file couldn't be removed. This is currently true for debugfs but not for procfs (by design). > * mmap is a big can of worms in that respect, mostly in terms of desired > semantics. What should be done to existing mappings when object is removed? > Sure, no new ones should be allowed, that much is obvious. But what should > happen to VMAs already there and what should happen to pages covered by > those? IMO the effect of truncate() on shared file mappings is the best > approximation for what we want there, but there's a considerable amount of > PITA in the vm_operations_struct that needs to be sorted out first. In > particular, ->open() and ->close() are often badly racy *and* there are > all kinds of bogisities with code making assumptions about impossibility of > getting several VMAs over the same object (fork() isn't the only thing that > could lead to that; munmap() the middle will do it as well and VM_DONTCOPY > won't prevent the latter). I'm not familiar with mm/ code but vma->vm_file holds a reference to the file which has been used by mmap(), which seems to mean that so long a mapping exists, the file is guaranteed not to be released. This assumption is useful when the lifetime of the object used by mmap() can be controlled, for example when it simply is allocated memory. If this lifetime can't be controlled and a mapped object is removed when mappings still exist, I hope there is a way to trigger the vm_ops->fault handler to give the module some control over resulting faults (I haven't checked the code to see how drivers handle such a situation). Of course, this works only if the module providing vma->vm_ops and vma->vm_ops->fault is still loaded in memory, which is not guaranteed as the vma does not take a reference to the module. This is currently not an issue because modules which implement ->mmap() use filesystems where "vma->vm_file = get_file(file)" (in mmap_region [6]) also prevents the module from being unloaded. Another possibility consists in making a module close every vma which has a ->vm_ops in it when it is unloaded, which seems quite hard to do in a safe way and useless for the modules which are currently in the kernel tree. > * as it is, mmap() in procfs has _very_ limited use - /proc/bus/pci/*/* > is all there, it's never modular and it never uses any d
Invalid assignment to gate in st21nfca_get_iso14443_3_uid
Hello, While compiling Linux with -Wunused-but-set-parameter, gcc reported a warning in st21nfca_get_iso14443_3_uid function, about "gate" being set but not used [1]. By looking at the code, it is clear that "gate = uid_skb->data;" does nothing useful. The function is only called once, by st21nfca_hci_target_from_gate [2], which uses the content of the uid array which is NOT initialized by st21nfca_get_iso14443_3_uid as the source of a memcpy [3]. My understanding of the code is that "gate = uid_skb->data;" in st21nfca_get_iso14443_3_uid should be changed to "memcpy(gate, uid_skb->data, uid_skb->len);". I'm new to the NFC subsystem so I can be wrong, in which case please tell me what I missed in my analysis. Please Cc me when replying as I am not subscribed to the mailing lists. Thanks, Nicolas [1] http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/nfc/st21nfca/st21nfca.c?id=v4.0-rc1#n575 [2] http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/nfc/st21nfca/st21nfca.c?id=v4.0-rc1#n646 [3] http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/nfc/st21nfca/st21nfca.c?id=v4.0-rc1#n682 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Bluetooth: always-true condition in dtl1_confcheck
Hello,
Since commit 00990e7ce0b0 ("pcmcia: use autoconfiguration feature for
ioports and iomem") function dtl1_confcheck() starts with the following
statements (in drivers/bluetooth/dtl1_cs.c):
static int dtl1_confcheck(struct pcmcia_device *p_dev, void *priv_data)
{
if ((p_dev->resource[1]->end) || (p_dev->resource[1]->end < 8))
return -ENODEV;
The condition in the if statement is always true and the compiler
reports an issue when building with -Wlogical-op. What was the condition
supposed to be?
Thanks,
Nicolas
qla3xxx: u32 constant overflow in fm93c56a_select()
Hello, In drivers/net/ethernet/qlogic/qla3xxx.c, fm93c56a_select() currently calls: ql_write_nvram_reg(qdev, spir, ((ISP_NVRAM_MASK << 16) | qdev->eeprom_cmd_data)); and ISP_NVRAM_MASK is defined as (0x000F << 16). ql_write_nvram_reg() writes a 32-bit value but (ISP_NVRAM_MASK << 16) expands to ((0x000F << 16) << 16) = 0xF << 32, which overflows the u32 type. This means the above call is equivalent to: ql_write_nvram_reg(qdev, spir, qdev->eeprom_cmd_data); Is this something normal, in which case (ISP_NVRAM_MASK << 16) would be removed, or a bug? Thanks, Nicolas
[PATCH 1/1] crypto: img-hash - use dma_data_direction when calling dma_map_sg
The fourth argument of dma_map_sg() and dma_unmap_sg() is an item of
dma_data_direction enum. Function img_hash_xmit_dma() wrongly used
DMA_MEM_TO_DEV, which is an item of dma_transfer_direction enum.
Replace DMA_MEM_TO_DEV (which value is 1) with DMA_TO_DEVICE (which
value is fortunately also 1) when calling dma_map_sg() and
dma_unmap_sg().
Signed-off-by: Nicolas Iooss
---
drivers/crypto/img-hash.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/crypto/img-hash.c b/drivers/crypto/img-hash.c
index a2e77b87485b..9b07f3d88feb 100644
--- a/drivers/crypto/img-hash.c
+++ b/drivers/crypto/img-hash.c
@@ -226,7 +226,7 @@ static int img_hash_xmit_dma(struct img_hash_dev *hdev,
struct scatterlist *sg)
struct dma_async_tx_descriptor *desc;
struct img_hash_request_ctx *ctx = ahash_request_ctx(hdev->req);
- ctx->dma_ct = dma_map_sg(hdev->dev, sg, 1, DMA_MEM_TO_DEV);
+ ctx->dma_ct = dma_map_sg(hdev->dev, sg, 1, DMA_TO_DEVICE);
if (ctx->dma_ct == 0) {
dev_err(hdev->dev, "Invalid DMA sg\n");
hdev->err = -EINVAL;
@@ -241,7 +241,7 @@ static int img_hash_xmit_dma(struct img_hash_dev *hdev,
struct scatterlist *sg)
if (!desc) {
dev_err(hdev->dev, "Null DMA descriptor\n");
hdev->err = -EINVAL;
- dma_unmap_sg(hdev->dev, sg, 1, DMA_MEM_TO_DEV);
+ dma_unmap_sg(hdev->dev, sg, 1, DMA_TO_DEVICE);
return -EINVAL;
}
desc->callback = img_hash_dma_callback;
--
2.11.0
[PATCH 1/1] ASoC: fsl_asrc: use dma_transfer_direction enum when calling dmaengine_prep_dma_cyclic
When fsl_asrc_dma_prepare_and_submit() calls
dmaengine_prep_dma_cyclic(), it uses either DMA_TO_DEVICE or
DMA_FROM_DEVICE. These values are both items of dma_data_direction enum,
not of dma_data_direction enum, which is the type expected by
dmaengine_prep_dma_cyclic().
Replace DMA_TO_DEVICE with DMA_MEM_TO_DEV and DMA_FROM_DEVICE with
DMA_DEV_TO_MEM to fix this type mismatch issue.
Signed-off-by: Nicolas Iooss
---
sound/soc/fsl/fsl_asrc_dma.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/fsl/fsl_asrc_dma.c b/sound/soc/fsl/fsl_asrc_dma.c
index dc30d780f874..282d841840b1 100644
--- a/sound/soc/fsl/fsl_asrc_dma.c
+++ b/sound/soc/fsl/fsl_asrc_dma.c
@@ -76,7 +76,7 @@ static int fsl_asrc_dma_prepare_and_submit(struct
snd_pcm_substream *substream)
pair->dma_chan[!dir], runtime->dma_addr,
snd_pcm_lib_buffer_bytes(substream),
snd_pcm_lib_period_bytes(substream),
- dir == OUT ? DMA_TO_DEVICE : DMA_FROM_DEVICE, flags);
+ dir == OUT ? DMA_MEM_TO_DEV : DMA_DEV_TO_MEM, flags);
if (!pair->desc[!dir]) {
dev_err(dev, "failed to prepare slave DMA for Front-End\n");
return -ENOMEM;
--
2.11.0
Re: [PATCH v3 1/1] x86, relocs: add printf attribute to die()
Hello,
As I have not received any comment on the patch I sent in December, I am
wondering whether I did anything wrong with it. How can I get it queued
for the next merge window?
Thanks,
Nicolas
On 18/12/16 20:47, Nicolas Iooss wrote:
> Adding such an attribute helps to detect errors in the format string at
> build time. After doing this, the compiler complains about such issues:
>
> arch/x86/tools/relocs.c:460:5: error: format specifies type 'int'
> but the argument has type 'Elf64_Xword' (aka 'unsigned long')
> [-Werror,-Wformat]
> sec->shdr.sh_size);
> ^
> arch/x86/tools/relocs.c:464:5: error: format specifies type 'int'
> but the argument has type 'Elf64_Off' (aka 'unsigned long')
> [-Werror,-Wformat]
> sec->shdr.sh_offset, strerror(errno));
> ^~~
>
> When relocs.c is included by relocs_32.c, sec->shdr.sh_size and
> sec->shdr.sh_offset are 32-bit unsigned integers. When the file is
> included by relocs_64.c, these expressions are 64-bit unsigned integers.
>
> Introduce a PRIuELF macro to define the right format to use when
> printing sh_size and sh_offset values.
>
> While at it, constify the format attribute of die().
>
> Signed-off-by: Nicolas Iooss
> ---
> I sent the first versions of this patch in September (cf.
> https://patchwork.kernel.org/patch/9312665/) but it has not been
> applied.
>
> As commit adee8705d251 ("x86/build: Annotate die() with noreturn to fix
> build warning on clang") introduced the noreturn attribute to die(),
> this patch now only adds the printf attribute.
>
> arch/x86/tools/relocs.c| 14 +++---
> arch/x86/tools/relocs.h| 3 ++-
> arch/x86/tools/relocs_32.c | 3 +++
> arch/x86/tools/relocs_64.c | 3 +++
> arch/x86/tools/relocs_common.c | 2 +-
> 5 files changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/arch/x86/tools/relocs.c b/arch/x86/tools/relocs.c
> index 0c2fae8d929d..4cad603b8d58 100644
> --- a/arch/x86/tools/relocs.c
> +++ b/arch/x86/tools/relocs.c
> @@ -397,7 +397,7 @@ static void read_shdrs(FILE *fp)
> ehdr.e_shnum);
> }
> if (fseek(fp, ehdr.e_shoff, SEEK_SET) < 0) {
> - die("Seek to %d failed: %s\n",
> + die("Seek to %"PRIuELF" failed: %s\n",
> ehdr.e_shoff, strerror(errno));
> }
> for (i = 0; i < ehdr.e_shnum; i++) {
> @@ -431,11 +431,11 @@ static void read_strtabs(FILE *fp)
> }
> sec->strtab = malloc(sec->shdr.sh_size);
> if (!sec->strtab) {
> - die("malloc of %d bytes for strtab failed\n",
> + die("malloc of %"PRIuELF" bytes for strtab failed\n",
> sec->shdr.sh_size);
> }
> if (fseek(fp, sec->shdr.sh_offset, SEEK_SET) < 0) {
> - die("Seek to %d failed: %s\n",
> + die("Seek to %"PRIuELF" failed: %s\n",
> sec->shdr.sh_offset, strerror(errno));
> }
> if (fread(sec->strtab, 1, sec->shdr.sh_size, fp)
> @@ -456,11 +456,11 @@ static void read_symtabs(FILE *fp)
> }
> sec->symtab = malloc(sec->shdr.sh_size);
> if (!sec->symtab) {
> - die("malloc of %d bytes for symtab failed\n",
> + die("malloc of %"PRIuELF" bytes for symtab failed\n",
> sec->shdr.sh_size);
> }
> if (fseek(fp, sec->shdr.sh_offset, SEEK_SET) < 0) {
> - die("Seek to %d failed: %s\n",
> + die("Seek to %"PRIuELF" failed: %s\n",
> sec->shdr.sh_offset, strerror(errno));
> }
> if (fread(sec->symtab, 1, sec->shdr.sh_size, fp)
> @@ -489,11 +489,11 @@ static void read_relocs(FILE *fp)
> }
> sec->reltab = malloc(sec->shdr.sh_size);
> if (!sec->reltab) {
> - die("malloc of %d bytes for relocs failed\n",
> + die("malloc of %"PRIuELF" bytes for relocs failed\n",
> sec->shdr.sh_size);
> }
&
Re: [PATCH v3 1/1] x86, relocs: add printf attribute to die()
On 01/02/17 10:04, Ingo Molnar wrote:
>
> * Nicolas Iooss wrote:
>
>> Adding such an attribute helps to detect errors in the format string at
>> build time. After doing this, the compiler complains about such issues:
>>
>> arch/x86/tools/relocs.c:460:5: error: format specifies type 'int'
>> but the argument has type 'Elf64_Xword' (aka 'unsigned long')
>> [-Werror,-Wformat]
>> sec->shdr.sh_size);
>> ^
>> arch/x86/tools/relocs.c:464:5: error: format specifies type 'int'
>> but the argument has type 'Elf64_Off' (aka 'unsigned long')
>> [-Werror,-Wformat]
>> sec->shdr.sh_offset, strerror(errno));
>> ^~~
>>
>> When relocs.c is included by relocs_32.c, sec->shdr.sh_size and
>> sec->shdr.sh_offset are 32-bit unsigned integers. When the file is
>> included by relocs_64.c, these expressions are 64-bit unsigned integers.
>>
>> Introduce a PRIuELF macro to define the right format to use when
>> printing sh_size and sh_offset values.
>>
>> While at it, constify the format attribute of die().
>>
>> Signed-off-by: Nicolas Iooss
>> ---
>> I sent the first versions of this patch in September (cf.
>> https://patchwork.kernel.org/patch/9312665/) but it has not been
>> applied.
>>
>> As commit adee8705d251 ("x86/build: Annotate die() with noreturn to fix
>> build warning on clang") introduced the noreturn attribute to die(),
>> this patch now only adds the printf attribute.
>>
>> arch/x86/tools/relocs.c| 14 +++---
>> arch/x86/tools/relocs.h| 3 ++-
>> arch/x86/tools/relocs_32.c | 3 +++
>> arch/x86/tools/relocs_64.c | 3 +++
>> arch/x86/tools/relocs_common.c | 2 +-
>> 5 files changed, 16 insertions(+), 9 deletions(-)
>>
>> diff --git a/arch/x86/tools/relocs.c b/arch/x86/tools/relocs.c
>> index 0c2fae8d929d..4cad603b8d58 100644
>> --- a/arch/x86/tools/relocs.c
>> +++ b/arch/x86/tools/relocs.c
>> @@ -397,7 +397,7 @@ static void read_shdrs(FILE *fp)
>> ehdr.e_shnum);
>> }
>> if (fseek(fp, ehdr.e_shoff, SEEK_SET) < 0) {
>> -die("Seek to %d failed: %s\n",
>> +die("Seek to %"PRIuELF" failed: %s\n",
>
> Doesn't a simple "%Ld" work as well?
With %Ld, my compiler (gcc 6.3.1 on x86_64) complains:
arch/x86/tools/relocs.c:400:7: error: format ‘%Ld’ expects argument of
type ‘long long int’, but argument 2 has type ‘Elf64_Off {aka long
unsigned int}’ [-Werror=format=]
Thanks,
Nicolas
Re: [PATCH v3 1/1] x86, relocs: add printf attribute to die()
On 02/02/17 02:39, h...@zytor.com wrote:
> On January 31, 2017 10:52:11 AM PST, Nicolas Iooss
> wrote:
>> Hello,
>>
>> As I have not received any comment on the patch I sent in December, I
>> am
>> wondering whether I did anything wrong with it. How can I get it queued
>> for the next merge window?
>>
>> Thanks,
>> Nicolas
>>
>> On 18/12/16 20:47, Nicolas Iooss wrote:
>>> Adding such an attribute helps to detect errors in the format string
>> at
>>> build time. After doing this, the compiler complains about such
>> issues:
>>>
>>> arch/x86/tools/relocs.c:460:5: error: format specifies type 'int'
>>> but the argument has type 'Elf64_Xword' (aka 'unsigned long')
>>> [-Werror,-Wformat]
>>> sec->shdr.sh_size);
>>> ^
>>> arch/x86/tools/relocs.c:464:5: error: format specifies type 'int'
>>> but the argument has type 'Elf64_Off' (aka 'unsigned long')
>>> [-Werror,-Wformat]
>>> sec->shdr.sh_offset,
>> strerror(errno));
>>> ^~~
>>>
>>> When relocs.c is included by relocs_32.c, sec->shdr.sh_size and
>>> sec->shdr.sh_offset are 32-bit unsigned integers. When the file is
>>> included by relocs_64.c, these expressions are 64-bit unsigned
>> integers.
>>>
>>> Introduce a PRIuELF macro to define the right format to use when
>>> printing sh_size and sh_offset values.
>>>
>>> While at it, constify the format attribute of die().
>>>
>>> Signed-off-by: Nicolas Iooss
>>> ---
>>> I sent the first versions of this patch in September (cf.
>>> https://patchwork.kernel.org/patch/9312665/) but it has not been
>>> applied.
>>>
>>> As commit adee8705d251 ("x86/build: Annotate die() with noreturn to
>> fix
>>> build warning on clang") introduced the noreturn attribute to die(),
>>> this patch now only adds the printf attribute.
>>>
>>> arch/x86/tools/relocs.c| 14 +++---
>>> arch/x86/tools/relocs.h| 3 ++-
>>> arch/x86/tools/relocs_32.c | 3 +++
>>> arch/x86/tools/relocs_64.c | 3 +++
>>> arch/x86/tools/relocs_common.c | 2 +-
>>> 5 files changed, 16 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/arch/x86/tools/relocs.c b/arch/x86/tools/relocs.c
>>> index 0c2fae8d929d..4cad603b8d58 100644
>>> --- a/arch/x86/tools/relocs.c
>>> +++ b/arch/x86/tools/relocs.c
>>> @@ -397,7 +397,7 @@ static void read_shdrs(FILE *fp)
>>> ehdr.e_shnum);
>>> }
>>> if (fseek(fp, ehdr.e_shoff, SEEK_SET) < 0) {
>>> - die("Seek to %d failed: %s\n",
>>> + die("Seek to %"PRIuELF" failed: %s\n",
>>> ehdr.e_shoff, strerror(errno));
>>> }
>>> for (i = 0; i < ehdr.e_shnum; i++) {
>>> @@ -431,11 +431,11 @@ static void read_strtabs(FILE *fp)
>>> }
>>> sec->strtab = malloc(sec->shdr.sh_size);
>>> if (!sec->strtab) {
>>> - die("malloc of %d bytes for strtab failed\n",
>>> + die("malloc of %"PRIuELF" bytes for strtab failed\n",
>>> sec->shdr.sh_size);
>>> }
>>> if (fseek(fp, sec->shdr.sh_offset, SEEK_SET) < 0) {
>>> - die("Seek to %d failed: %s\n",
>>> + die("Seek to %"PRIuELF" failed: %s\n",
>>> sec->shdr.sh_offset, strerror(errno));
>>> }
>>> if (fread(sec->strtab, 1, sec->shdr.sh_size, fp)
>>> @@ -456,11 +456,11 @@ static void read_symtabs(FILE *fp)
>>> }
>>> sec->symtab = malloc(sec->shdr.sh_size);
>>> if (!sec->symtab) {
>>> - die("malloc of %d bytes for symtab failed\n",
>>> + die("malloc of %"PRIuELF" bytes for symtab failed\n",
>>> sec->shdr.sh_size);
>>> }
>>> if (fseek(fp, sec->shdr.sh_offset, SEEK_SET) <
[PATCH 1/1] IB/cxgb3: fix misspelling in header guard
Use CXGB3_... instead of CXBG3_... Signed-off-by: Nicolas Iooss --- include/uapi/rdma/cxgb3-abi.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/uapi/rdma/cxgb3-abi.h b/include/uapi/rdma/cxgb3-abi.h index 48a19bda071b..d24eee12128f 100644 --- a/include/uapi/rdma/cxgb3-abi.h +++ b/include/uapi/rdma/cxgb3-abi.h @@ -30,7 +30,7 @@ * SOFTWARE. */ #ifndef CXGB3_ABI_USER_H -#define CXBG3_ABI_USER_H +#define CXGB3_ABI_USER_H #include -- 2.11.0
[PATCH 1/1] drm/amd/powerplay: fix misspelling in header guard
In smu7_clockpowergating.h, the #ifndef statement which prevents multiple inclusions of the header file uses _SMU7_CLOCK_POWER_GATING_H_ but the following #define statement uses _SMU7_CLOCK__POWER_GATING_H_. Signed-off-by: Nicolas Iooss --- drivers/gpu/drm/amd/powerplay/hwmgr/smu7_clockpowergating.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_clockpowergating.h b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_clockpowergating.h index d52a28c343e3..c96ed9ed7eaf 100644 --- a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_clockpowergating.h +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_clockpowergating.h @@ -22,7 +22,7 @@ */ #ifndef _SMU7_CLOCK_POWER_GATING_H_ -#define _SMU7_CLOCK__POWER_GATING_H_ +#define _SMU7_CLOCK_POWER_GATING_H_ #include "smu7_hwmgr.h" #include "pp_asicblocks.h" -- 2.11.0
[PATCH 1/1] EDAC: always return an initialized value in knl_show_interleave_mode
When building drivers/edac/sb_edac.c with compiler warning flags which
aim to detect the use of uninitialized values at compile time, the
compiler reports that knl_show_interleave_mode() may return an
uninitialized value. This function indeed uses a switch statement to set
a local variable ("s"), which is not set to anything in the default
statement. Anyway this would be never reached as
knl_interleave_mode(reg) always returns a value between 0 and 3, but the
compiler has no way of knowing this.
Silent the compiler warning by initializing variable "s" too in the
default case. While at it, make knl_show_interleave_mode() and
show_interleave_mode() return const char* values as the returned
pointers refer to read-only memory.
Signed-off-by: Nicolas Iooss
---
drivers/edac/sb_edac.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/edac/sb_edac.c b/drivers/edac/sb_edac.c
index 54ae6dc45ab2..15a068744c25 100644
--- a/drivers/edac/sb_edac.c
+++ b/drivers/edac/sb_edac.c
@@ -304,7 +304,7 @@ struct sbridge_info {
u64 (*rir_limit)(u32 reg);
u64 (*sad_limit)(u32 reg);
u32 (*interleave_mode)(u32 reg);
- char* (*show_interleave_mode)(u32 reg);
+ const char* (*show_interleave_mode)(u32 reg);
u32 (*dram_attr)(u32 reg);
const u32 *dram_rule;
const u32 *interleave_list;
@@ -811,7 +811,7 @@ static u32 interleave_mode(u32 reg)
return GET_BITFIELD(reg, 1, 1);
}
-char *show_interleave_mode(u32 reg)
+static const char *show_interleave_mode(u32 reg)
{
return interleave_mode(reg) ? "8:6" : "[8:6]XOR[18:16]";
}
@@ -831,9 +831,9 @@ static u32 knl_interleave_mode(u32 reg)
return GET_BITFIELD(reg, 1, 2);
}
-static char *knl_show_interleave_mode(u32 reg)
+static const char *knl_show_interleave_mode(u32 reg)
{
- char *s;
+ const char *s;
switch (knl_interleave_mode(reg)) {
case 0:
@@ -850,6 +850,7 @@ static char *knl_show_interleave_mode(u32 reg)
break;
default:
WARN_ON(1);
+ s = "";
break;
}
--
2.11.0
Re: [PATCH 1/1] EDAC: always return an initialized value in knl_show_interleave_mode
On 22/01/17 15:41, Borislav Petkov wrote:
> On Sun, Jan 22, 2017 at 02:51:15PM +0100, Nicolas Iooss wrote:
>> When building drivers/edac/sb_edac.c with compiler warning flags which
>> aim to detect the use of uninitialized values at compile time, the
>> compiler reports that knl_show_interleave_mode() may return an
>> uninitialized value. This function indeed uses a switch statement to set
>> a local variable ("s"), which is not set to anything in the default
>> statement. Anyway this would be never reached as
>> knl_interleave_mode(reg) always returns a value between 0 and 3, but the
>> compiler has no way of knowing this.
>>
>> Silent the compiler warning by initializing variable "s" too in the
>> default case. While at it, make knl_show_interleave_mode() and
>> show_interleave_mode() return const char* values as the returned
>> pointers refer to read-only memory.
>
> No, this is trying to make pretty an already ugly pile of crap.
>
> That ->show_interleave_mode() is called only once in a debug printk. Big
> deal. But it is a function pointer which points to the same function
> except for KNL.
>
> Then, the default implementation returns bit slices: "8:6" :
> "[8:6]XOR[18:16]" but the KNL one says "use address bits" like this is
> the SDM. Yeah, right. I should've caught that when the KNL pile was
> added.
>
> So here's what I'd like you to do, instead:
>
> Kill that ->show_interleave_mode() thing and use the default
> show_interleave_mode() at the only call site. You can pass in a second
> bool argument is_knl which is "pvt->info.type == KNIGHTS_LANDING".
>
> And then add the KNL functionality from knl_show_interleave_mode() to
> the default show_interleave_mode().
>
> And get rid of that default: case - it won't be reached anyway.
>
> You can even define a string array:
>
> struct pair intlv_mode[] = {
> "[8:6]", "[10:8]", ...;
> };
>
> and then do:
>
> if (!is_knl && !interleave_mode(reg))
> return "[8:6]XOR[18:16]";
> else
> return intlv_mode[knl_interleave_mode()];
>
> and be done with it.
>
> Thanks.
Thanks for your quick reply! I agree with your proposal and will send an
other patch which implements it.
In your reply, there is one point I have not understood. When doing "if
(!is_knl && !interleave_mode(reg))", the condition assumes that
interleave mode 1 has the same meaning on all kinds of CPUs. However the
current code prints this mode as "[10:8]" on KNL and "8:6" anywhere
else. So I would rather modify the code this way:
if (!is_knl)
return interleave_mode(reg) ?
"[8:6]" : "[8:6]XOR[18:16]";
else
return knl_intlv_mode[knl_interleave_mode(reg)];
Would this be good for you?
Thanks
[PATCH v2 1/1] EDAC: kill ->show_interleave_mode()
Function sbridge_register_mci() sets pvt->info.show_interleave_mode to
knl_show_interleave_mode() on Knight's Landing and
show_interleave_mode() anywhere else. These functions are only called in
a debug statement and knl_show_interleave_mode() implementation causes a
compiler warning (the compiler fails to understand that the default
branch of the switch condition can never be reached because reg is a
2-bit value).
Merge show_interleave_mode() and knl_show_interleave_mode() in a single
implementation and use it without an indirect function pointer.
Signed-off-by: Nicolas Iooss
---
drivers/edac/sb_edac.c | 44 ++--
1 file changed, 10 insertions(+), 34 deletions(-)
diff --git a/drivers/edac/sb_edac.c b/drivers/edac/sb_edac.c
index 54ae6dc45ab2..60731979d030 100644
--- a/drivers/edac/sb_edac.c
+++ b/drivers/edac/sb_edac.c
@@ -304,7 +304,6 @@ struct sbridge_info {
u64 (*rir_limit)(u32 reg);
u64 (*sad_limit)(u32 reg);
u32 (*interleave_mode)(u32 reg);
- char* (*show_interleave_mode)(u32 reg);
u32 (*dram_attr)(u32 reg);
const u32 *dram_rule;
const u32 *interleave_list;
@@ -811,11 +810,6 @@ static u32 interleave_mode(u32 reg)
return GET_BITFIELD(reg, 1, 1);
}
-char *show_interleave_mode(u32 reg)
-{
- return interleave_mode(reg) ? "8:6" : "[8:6]XOR[18:16]";
-}
-
static u32 dram_attr(u32 reg)
{
return GET_BITFIELD(reg, 2, 3);
@@ -831,29 +825,16 @@ static u32 knl_interleave_mode(u32 reg)
return GET_BITFIELD(reg, 1, 2);
}
-static char *knl_show_interleave_mode(u32 reg)
-{
- char *s;
-
- switch (knl_interleave_mode(reg)) {
- case 0:
- s = "use address bits [8:6]";
- break;
- case 1:
- s = "use address bits [10:8]";
- break;
- case 2:
- s = "use address bits [14:12]";
- break;
- case 3:
- s = "use address bits [32:30]";
- break;
- default:
- WARN_ON(1);
- break;
- }
+static const char * const knl_intlv_mode[] = {
+ "[8:6]", "[10:8]", "[14:12]", "[32:30]"
+};
- return s;
+static const char *show_interleave_mode(u32 reg, enum type type)
+{
+ if (type == KNIGHTS_LANDING)
+ return knl_intlv_mode[knl_interleave_mode(reg)];
+ else
+ return interleave_mode(reg) ? "[8:6]" : "[8:6]XOR[18:16]";
}
static u32 dram_attr_knl(u32 reg)
@@ -1810,7 +1791,7 @@ static void get_memory_layout(const struct mem_ctl_info
*mci)
show_dram_attr(pvt->info.dram_attr(reg)),
gb, (mb*1000)/1024,
((u64)tmp_mb) << 20L,
-pvt->info.show_interleave_mode(reg),
+show_interleave_mode(reg, pvt->info.type),
reg);
prv = limit;
@@ -3227,7 +3208,6 @@ static int sbridge_register_mci(struct sbridge_dev
*sbridge_dev, enum type type)
pvt->info.rir_limit = rir_limit;
pvt->info.sad_limit = sad_limit;
pvt->info.interleave_mode = interleave_mode;
- pvt->info.show_interleave_mode = show_interleave_mode;
pvt->info.dram_attr = dram_attr;
pvt->info.max_sad = ARRAY_SIZE(ibridge_dram_rule);
pvt->info.interleave_list = ibridge_interleave_list;
@@ -3251,7 +3231,6 @@ static int sbridge_register_mci(struct sbridge_dev
*sbridge_dev, enum type type)
pvt->info.rir_limit = rir_limit;
pvt->info.sad_limit = sad_limit;
pvt->info.interleave_mode = interleave_mode;
- pvt->info.show_interleave_mode = show_interleave_mode;
pvt->info.dram_attr = dram_attr;
pvt->info.max_sad = ARRAY_SIZE(sbridge_dram_rule);
pvt->info.interleave_list = sbridge_interleave_list;
@@ -3275,7 +3254,6 @@ static int sbridge_register_mci(struct sbridge_dev
*sbridge_dev, enum type type)
pvt->info.rir_limit = haswell_rir_limit;
pvt->info.sad_limit = sad_limit;
pvt->info.interleave_mode = interleave_mode;
- pvt->info.show_interleave_mode = show_interleave_mode;
pvt->info.dram_attr = dram_attr;
pvt->info.max_sad = ARRAY_SIZE(ibridge_dram_rule);
pvt->info.interleave_list = ibridge_interleave_list;
@@ -3299,7 +3277,6 @@ static int sbridge_register_mci(struct sbridge_dev
*sbridge_dev, enum type type)
pvt->info.rir_limit = haswell_rir_limit;
pvt
[PATCH 1/1] ath10k: use the right length of "background"
The word "background" contains 10 characters so the third argument of
strncmp() need to be 10 in order to match this prefix correctly.
Signed-off-by: Nicolas Iooss
Fixes: 855aed1220d2 ("ath10k: add spectral scan feature")
---
drivers/net/wireless/ath/ath10k/spectral.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ath/ath10k/spectral.c
b/drivers/net/wireless/ath/ath10k/spectral.c
index 7d9b0da1b010..2ffc1fe4923b 100644
--- a/drivers/net/wireless/ath/ath10k/spectral.c
+++ b/drivers/net/wireless/ath/ath10k/spectral.c
@@ -338,7 +338,7 @@ static ssize_t write_file_spec_scan_ctl(struct file *file,
} else {
res = -EINVAL;
}
- } else if (strncmp("background", buf, 9) == 0) {
+ } else if (strncmp("background", buf, 10) == 0) {
res = ath10k_spectral_scan_config(ar, SPECTRAL_BACKGROUND);
} else if (strncmp("manual", buf, 6) == 0) {
res = ath10k_spectral_scan_config(ar, SPECTRAL_MANUAL);
--
2.10.1
[PATCH 1/1] nvdimm: use the right length of "pmem"
In order to test that the name of a resource begins with "pmem", call
strncmp() with 4 as length instead of 3 to match the whole prefix.
Fixes: 16660eaea0cc ("libnvdimm, namespace: update label implementation
for multi-pmem")
Signed-off-by: Nicolas Iooss
---
drivers/nvdimm/label.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/nvdimm/label.c b/drivers/nvdimm/label.c
index fac7cabe8f56..dd615345699f 100644
--- a/drivers/nvdimm/label.c
+++ b/drivers/nvdimm/label.c
@@ -938,7 +938,7 @@ int nd_pmem_namespace_label_update(struct nd_region
*nd_region,
}
for_each_dpa_resource(ndd, res)
- if (strncmp(res->name, "pmem", 3) == 0)
+ if (strncmp(res->name, "pmem", 4) == 0)
count++;
WARN_ON_ONCE(!count);
--
2.10.1
[PATCH 1/1] ALSA: bebob: use the right string length in check_audiophile_booted()
Function check_audiophile_booted() only compares 15 characters of the
24-character-long string "FW Audiophile Bootloader" with the firmware
model name. As this seems to be incorrect and because there is no
comment explaining this "15", fix the length which is used in strncmp().
This patch has only been compile-tested.
Fixes: 9076c22ddd9d ("ALSA: bebob: Add support for M-Audio usual
Firewire series")
Signed-off-by: Nicolas Iooss
---
sound/firewire/bebob/bebob.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/firewire/bebob/bebob.c b/sound/firewire/bebob/bebob.c
index 3469ac14c89c..b23cd0901553 100644
--- a/sound/firewire/bebob/bebob.c
+++ b/sound/firewire/bebob/bebob.c
@@ -177,7 +177,7 @@ check_audiophile_booted(struct fw_unit *unit)
if (fw_csr_string(unit->directory, CSR_MODEL, name, sizeof(name)) < 0)
return false;
- return strncmp(name, "FW Audiophile Bootloader", 15) != 0;
+ return strncmp(name, "FW Audiophile Bootloader", 24) != 0;
}
static void
--
2.10.1
[PATCH 1/4] isdn/eicon: remove unused argument in DBG_ERR call
diva_um_idi_read() can call DBG_ERR with 3 format arguments but using a
format string which only uses 2 of them. Remove the last one.
This bug has been found by adding a __printf attribute to
myDbgPrint_...() functions. As this addition leads the compiler to
report a lot of -Wformat warnings (for example the compiler complains
when "%08x" is used to format a pointer, as it is done with all usages
of "E(%08x)" in um_idi.c), this patch does not add any __printf
attribute.
This patch has only been compile-tested.
Signed-off-by: Nicolas Iooss
---
drivers/isdn/hardware/eicon/um_idi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/isdn/hardware/eicon/um_idi.c
b/drivers/isdn/hardware/eicon/um_idi.c
index e1519718ce67..13ef38fa6cb0 100644
--- a/drivers/isdn/hardware/eicon/um_idi.c
+++ b/drivers/isdn/hardware/eicon/um_idi.c
@@ -351,7 +351,7 @@ int diva_um_idi_read(void *entity,
Not enough space to read message
*/
DBG_ERR(("A: A(%d) E(%08x) read small buffer",
-a->adapter_nr, e, ret));
+a->adapter_nr, e));
diva_os_leave_spin_lock(&adapter_lock, &old_irql,
"read");
return (-2);
--
2.10.1
[PATCH 4/4] isdn/eicon: use const strings with format arguments
Functions using a printf format argument do not modify the value of this
argument. These functions can therefore use type "const char *" instead
of "char *".
This patch has only been compile-tested.
Signed-off-by: Nicolas Iooss
---
drivers/isdn/hardware/eicon/debug.c| 14 +++---
drivers/isdn/hardware/eicon/debuglib.h | 6 +++---
drivers/isdn/hardware/eicon/maintidi.c | 2 +-
3 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/isdn/hardware/eicon/debug.c
b/drivers/isdn/hardware/eicon/debug.c
index cd8d70e3292d..b8772bbee872 100644
--- a/drivers/isdn/hardware/eicon/debug.c
+++ b/drivers/isdn/hardware/eicon/debug.c
@@ -14,9 +14,9 @@
static void DI_register(void *arg);
static void DI_deregister(pDbgHandle hDbg);
-static void DI_format(int do_lock, word id, int type, char *format, va_list
argument_list);
-static void DI_format_locked(word id, int type, char *format, va_list
argument_list);
-static void DI_format_old(word id, char *format, va_list ap) { }
+static void DI_format(int do_lock, word id, int type, const char *format,
va_list argument_list);
+static void DI_format_locked(word id, int type, const char *format, va_list
argument_list);
+static void DI_format_old(word id, const char *format, va_list ap) { }
static void DiProcessEventLog(unsigned short id, unsigned long msgID, va_list
ap) { }
static void single_p(byte *P, word *PLength, byte Id);
static void diva_maint_xdi_cb(ENTITY *e);
@@ -25,7 +25,7 @@ static int diva_mnt_cmp_nmbr(const char *nmbr);
static void diva_free_dma_descriptor(IDI_CALL request, int nr);
static int diva_get_dma_descriptor(IDI_CALL request, dword *dma_magic);
__printf(3, 4)
-void diva_mnt_internal_dprintf(dword drv_id, dword type, char *p, ...);
+void diva_mnt_internal_dprintf(dword drv_id, dword type, const char *p, ...);
static dword MaxDumpSize = 256;
static dword MaxXlogSize = 2 + 128;
@@ -561,7 +561,7 @@ static void DI_deregister(pDbgHandle hDbg) {
static void DI_format_locked(unsigned short id,
int type,
-char *format,
+const char *format,
va_list argument_list) {
DI_format(1, id, type, format, argument_list);
}
@@ -569,7 +569,7 @@ static void DI_format_locked(unsigned short id,
static void DI_format(int do_lock,
unsigned short id,
int type,
- char *format,
+ const char *format,
va_list ap) {
diva_os_spin_lock_magic_t old_irql;
dword sec, usec;
@@ -1904,7 +1904,7 @@ static void
diva_change_management_debug_mask(diva_maint_client_t *pC, dword old
}
-void diva_mnt_internal_dprintf(dword drv_id, dword type, char *fmt, ...) {
+void diva_mnt_internal_dprintf(dword drv_id, dword type, const char *fmt, ...)
{
va_list ap;
va_start(ap, fmt);
diff --git a/drivers/isdn/hardware/eicon/debuglib.h
b/drivers/isdn/hardware/eicon/debuglib.h
index 6dcbf6afb8f9..2170de140335 100644
--- a/drivers/isdn/hardware/eicon/debuglib.h
+++ b/drivers/isdn/hardware/eicon/debuglib.h
@@ -230,10 +230,10 @@ extern void DbgSetLevel(unsigned long dbgMask);
*/
typedef struct _DbgHandle_ *pDbgHandle;
typedef void (*DbgEnd)(pDbgHandle);
-typedef void (*DbgLog)(unsigned short, int, char *, va_list);
-typedef void (*DbgOld)(unsigned short, char *, va_list);
+typedef void (*DbgLog)(unsigned short, int, const char *, va_list);
+typedef void (*DbgOld)(unsigned short, const char *, va_list);
typedef void (*DbgEv)(unsigned short, unsigned long, va_list);
-typedef void (*DbgIrq)(unsigned short, int, char *, va_list);
+typedef void (*DbgIrq)(unsigned short, int, const char *, va_list);
typedef struct _DbgHandle_
{ charRegistered; /* driver successfully registered */
#define DBG_HANDLE_REG_NEW 0x01 /* this (new) structure*/
diff --git a/drivers/isdn/hardware/eicon/maintidi.c
b/drivers/isdn/hardware/eicon/maintidi.c
index b2ed2939b4fa..a635595e9be3 100644
--- a/drivers/isdn/hardware/eicon/maintidi.c
+++ b/drivers/isdn/hardware/eicon/maintidi.c
@@ -31,7 +31,7 @@
extern __printf(3, 4)
-void diva_mnt_internal_dprintf(dword drv_id, dword type, char *p, ...);
+void diva_mnt_internal_dprintf(dword drv_id, dword type, const char *p, ...);
#define MODEM_PARSE_ENTRIES 16 /* amount of variables of interest */
#define FAX_PARSE_ENTRIES12 /* amount of variables of interest */
--
2.10.1
[PATCH 2/4] isdn/eicon: fix some message formatting errors
There are some inconsistent debug message formats in message.c. For
example,
dprintf("XDI CAPI: RC cancelled Id:0x02, Ch:%02x", e->Id, ch);
wrongly reports an ID of 2 and prints the entity ID as the channel ID.
There are also object pointers which are used instead of the IDs.
All these inconsistent formats have been found by adding __printf
attribute to myDbgPrint_...() functions (used by dbug()). As this makes
the compiler to also complain about using "%ld" with unsigned int values
(instead of "%u") and some other less-important format issues, this
patch does not add any __printf attribute.
This patch has only been compile-tested.
Signed-off-by: Nicolas Iooss
---
drivers/isdn/hardware/eicon/message.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/isdn/hardware/eicon/message.c
b/drivers/isdn/hardware/eicon/message.c
index 1a1d99704fe6..7cafa34c3464 100644
--- a/drivers/isdn/hardware/eicon/message.c
+++ b/drivers/isdn/hardware/eicon/message.c
@@ -1059,7 +1059,7 @@ static void plci_remove(PLCI *plci)
}
if (plci->Sig.Id == 0xff)
{
- dbug(1, dprintf("D-channel X.25 plci->NL.Id:%0x", plci->NL.Id));
+ dbug(1, dprintf("D-channel X.25 plci->NL.Id:%02x",
plci->NL.Id));
if (plci->NL.Id && !plci->nl_remove_id)
{
nl_req_ncci(plci, REMOVE, 0);
@@ -3109,7 +3109,7 @@ static byte data_b3_req(dword Id, word Number,
DIVA_CAPI_ADAPTER *a,
Info = _WRONG_IDENTIFIER;
ncci = (word)(Id >> 16);
- dbug(1, dprintf("ncci=0x%x, plci=0x%x", ncci, plci));
+ dbug(1, dprintf("ncci=0x%x, plci=0x%x", ncci, plci->Id));
if (plci && ncci)
{
@@ -3325,7 +3325,7 @@ static byte select_b_req(dword Id, word Number,
DIVA_CAPI_ADAPTER *a,
else
{
dbug(1,
dprintf("select_b_req[%d],PLCI=0x%x,Tel=0x%x,NL=0x%x,appl=0x%x,sstate=0x%x",
- msg->length, plci->Id, plci->tel, plci->NL.Id,
plci->appl, plci->SuppState));
+ msg->length, plci->Id, plci->tel, plci->NL.Id,
appl->Id, plci->SuppState));
dbug(1, dprintf("PlciState=0x%x", plci->State));
for (i = 0; i < 7; i++) bp_parms[i].length = 0;
@@ -3910,7 +3910,7 @@ void callback(ENTITY *e)
if (no_cancel_rc && (a->FlowControlIdTable[ch]
== e->Id) && e->Id) {
a->FlowControlIdTable[ch] = 0;
if ((rc == OK) &&
a->FlowControlSkipTable[ch]) {
- dbug(3, dprintf("XDI CAPI: RC
cancelled Id:0x02, Ch:%02x", e->Id, ch));
+ dbug(3, dprintf("XDI CAPI: RC
cancelled Id:%02x, Ch:%02x", e->Id, ch));
return;
}
}
@@ -9135,7 +9135,7 @@ static word AdvCodecSupport(DIVA_CAPI_ADAPTER *a, PLCI
*plci, APPL *appl,
{
if (a->AdvSignalAppl != appl || a->AdvSignalPLCI)
{
- dbug(1, dprintf("AdvSigPlci=0x%x",
a->AdvSignalPLCI));
+ dbug(1, dprintf("AdvSigPlci=0x%x",
a->AdvSignalPLCI->Id));
return 0x2001; /* codec in use by another
application */
}
if (plci != NULL)
--
2.10.1
[PATCH 3/4] isdn/eicon: add some __printf attributes
Add __printf attributes to some functions. This helps detecting errors related to printf-formats at compile time. When doing this, gcc reports some issues in debug.c. Fix them. This patch has only been compile-tested. Signed-off-by: Nicolas Iooss --- drivers/isdn/hardware/eicon/debug.c| 129 + drivers/isdn/hardware/eicon/maintidi.c | 3 +- drivers/isdn/hardware/eicon/platform.h | 2 +- 3 files changed, 68 insertions(+), 66 deletions(-) diff --git a/drivers/isdn/hardware/eicon/debug.c b/drivers/isdn/hardware/eicon/debug.c index 576b7b4a3278..cd8d70e3292d 100644 --- a/drivers/isdn/hardware/eicon/debug.c +++ b/drivers/isdn/hardware/eicon/debug.c @@ -24,6 +24,7 @@ static word SuperTraceCreateReadReq(byte *P, const char *path); static int diva_mnt_cmp_nmbr(const char *nmbr); static void diva_free_dma_descriptor(IDI_CALL request, int nr); static int diva_get_dma_descriptor(IDI_CALL request, dword *dma_magic); +__printf(3, 4) void diva_mnt_internal_dprintf(dword drv_id, dword type, char *p, ...); static dword MaxDumpSize = 256; @@ -1514,29 +1515,29 @@ static void diva_maint_state_change_notify(void *user_context, } - diva_mnt_internal_dprintf(pC->hDbg->id, DLI_STAT, "MDM Ch= %lu", + diva_mnt_internal_dprintf(pC->hDbg->id, DLI_STAT, "MDM Ch= %u", (int)modem->ChannelNumber); - diva_mnt_internal_dprintf(pC->hDbg->id, DLI_STAT, "MDM Event = %lu", modem->Event); - diva_mnt_internal_dprintf(pC->hDbg->id, DLI_STAT, "MDM Norm = %lu", modem->Norm); + diva_mnt_internal_dprintf(pC->hDbg->id, DLI_STAT, "MDM Event = %u", modem->Event); + diva_mnt_internal_dprintf(pC->hDbg->id, DLI_STAT, "MDM Norm = %u", modem->Norm); diva_mnt_internal_dprintf(pC->hDbg->id, DLI_STAT, "MDM Opts. = 0x%08x", modem->Options); - diva_mnt_internal_dprintf(pC->hDbg->id, DLI_STAT, "MDM Tx= %lu Bps", modem->TxSpeed); - diva_mnt_internal_dprintf(pC->hDbg->id, DLI_STAT, "MDM Rx= %lu Bps", modem->RxSpeed); - diva_mnt_internal_dprintf(pC->hDbg->id, DLI_STAT, "MDM RT= %lu mSec", + diva_mnt_internal_dprintf(pC->hDbg->id, DLI_STAT, "MDM Tx= %u Bps", modem->TxSpeed); + diva_mnt_internal_dprintf(pC->hDbg->id, DLI_STAT, "MDM Rx= %u Bps", modem->RxSpeed); + diva_mnt_internal_dprintf(pC->hDbg->id, DLI_STAT, "MDM RT= %u mSec", modem->RoundtripMsec); - diva_mnt_internal_dprintf(pC->hDbg->id, DLI_STAT, "MDM Sr= %lu", modem->SymbolRate); + diva_mnt_internal_dprintf(pC->hDbg->id, DLI_STAT, "MDM Sr= %u", modem->SymbolRate); diva_mnt_internal_dprintf(pC->hDbg->id, DLI_STAT, "MDM Rxl = %d dBm", modem->RxLeveldBm); diva_mnt_internal_dprintf(pC->hDbg->id, DLI_STAT, "MDM El= %d dBm", modem->EchoLeveldBm); - diva_mnt_internal_dprintf(pC->hDbg->id, DLI_STAT, "MDM SNR = %lu dB", modem->SNRdb); - diva_mnt_internal_dprintf(pC->hDbg->id, DLI_STAT, "MDM MAE = %lu", modem->MAE); - diva_mnt_internal_dprintf(pC->hDbg->id, DLI_STAT, "MDM LRet = %lu", + diva_mnt_internal_dprintf(pC->hDbg->id, DLI_STAT, "MDM SNR = %u dB", modem->SNRdb); + diva_mnt_internal_dprintf(pC->hDbg->id, DLI_STAT, "MDM MAE = %u", modem->MAE); + diva_mnt_internal_dprintf(pC->hDbg->id, DLI_STAT, "MDM LRet = %u", modem->LocalRetrains); - diva_mnt_internal_dprintf(pC->hDbg->id, DLI_STAT, "MDM RRet = %lu", + diva_mnt_internal_dprintf(pC->hDbg->id, DLI_STAT, "MDM RRet = %u", modem->RemoteRetrains); - diva_mnt_internal_dprintf(pC->hDbg->id, DLI_STAT, "MDM LRes = %lu", modem->LocalResyncs); - diva_mnt_internal_dprintf(pC->hDbg->id, DLI_STAT, "MDM RRes = %lu", + diva_mnt_internal_dprintf(pC->hDbg->id, DLI_
Re: sound: bebop: strncmp length oddity
On 17/11/16 21:07, Takashi Sakamoto wrote:
> On Nov 17 2016 20:47, Takashi Iwai wrote:
>> On Sat, 29 Oct 2016 23:37:00 +0200,
>> Joe Perches wrote:
>>>
>>> 15 isn't the length of the string, is that really what's desired?
>>>
>>> linux/next/sound/firewire/bebob/bebop.c
>>>
>>> -
>>>
>>> static bool
>>> check_audiophile_booted(struct fw_unit *unit)
>>> {
>>> char name[24] = {0};
>>>
>>> if (fw_csr_string(unit->directory, CSR_MODEL, name, sizeof(name))
>>> < 0)
>>> return false;
>>>
>>> return strncmp(name, "FW Audiophile Bootloader", 15) != 0;
>>> }
>>
>> Indeed it looks bogus. Even "FW" string is already 24 letters, so
>> it's over name[] array size.
>>
>> Sakamoto-san, could you fix it properly?
>
> It's OK just to compare first a few characters, while I left whole
> string for our information because model-specific information is easily
> lost.
>
> For M-Audio's FireWire Audiophile, modalias of
> 'ieee1394:ven0D6Cmo00010060sp' hits this unit only. However the unit
> has two states relevant to loaded firmware. Initial firmware returns 'FW
> Audiophile Bootloader', while functional firmware returns 'FW
> Audiophile'. Just comparing the first 15 characters is the shortest way
> to defferentiate them. (Actually 14 characters. I guess that I did avoid
> comparison based on LWS...)
>
> Therefore, changing comparison range is not for bug fix, just for
> readers. But I agree with these patches because the code is a bit
> confusing. I'll post a patch soon for this aim with appropriate
> information, sorry for Nicolas.
All right, so long as the code is modified in a way that makes it more
consistent, I am all right. It is one patch less I need to care about :)
Nicolas
Re: [PATCH v2 1/1] x86, relocs: add function attributes to die()
Hello,
A few weeks ago I sent the patch below and got a review, but this patch
has not been applied yet in linux-next. Does it cause a problem I need
to fix?
Thanks,
Nicolas
On 05/09/16 17:28, Nilay Vaish wrote:
> On 4 September 2016 at 11:51, Nicolas Iooss
> wrote:
>> When building the kernel with clang and some warning flags, the compiler
>> reports the following warning:
>>
>> arch/x86/tools/relocs.c:979:6: warning: variable 'do_reloc' is used
>> uninitialized whenever 'if' condition is false
>> [-Wsometimes-uninitialized]
>> if (!use_real_mode)
>> ^~
>> arch/x86/tools/relocs.c:991:14: note: uninitialized use occurs here
>> walk_relocs(do_reloc);
>> ^~~~
>> arch/x86/tools/relocs.c:979:2: note: remove the 'if' if its
>> condition is always true
>> if (!use_real_mode)
>> ^~~
>> arch/x86/tools/relocs.c:976:24: note: initialize the variable
>> 'do_reloc' to silence this warning
>> const char *symname);
>> ^
>> = NULL
>>
>> This is obviously a false positive: whenever the 'if' condition is
>> false, the program calls die(). Nevertheless the compiler did not know
>> this call makes the program quit because this function did not have a
>> noreturn attribute. Add it.
>>
>> While at it, add a printf attribute too to die() and constify the format
>> parameter. This leads to some errors when compiling on x86_64:
>>
>> arch/x86/tools/relocs.c:460:5: error: format specifies type 'int'
>> but the argument has type 'Elf64_Xword' (aka 'unsigned long')
>> [-Werror,-Wformat]
>> sec->shdr.sh_size);
>> ^
>> arch/x86/tools/relocs.c:464:5: error: format specifies type 'int'
>> but the argument has type 'Elf64_Off' (aka 'unsigned long')
>> [-Werror,-Wformat]
>> sec->shdr.sh_offset, strerror(errno));
>> ^~~~~~~~~~~
>>
>> When relocs.c is included by relocs_32.c, sec->shdr.sh_size and
>> sec->shdr.sh_offset are 32-bit unsigned integers. When the file is
>> included by relocs_64.c, these expressions are 64-bit unsigned integers.
>>
>> Introduce a PRIuELF macro to define the right format to use when
>> printing these expressions.
>>
>> Signed-off-by: Nicolas Iooss
>> ---
>> arch/x86/tools/relocs.c| 14 +++---
>> arch/x86/tools/relocs.h| 3 ++-
>> arch/x86/tools/relocs_32.c | 3 +++
>> arch/x86/tools/relocs_64.c | 3 +++
>> arch/x86/tools/relocs_common.c | 2 +-
>> 5 files changed, 16 insertions(+), 9 deletions(-)
>>
>> diff --git a/arch/x86/tools/relocs.c b/arch/x86/tools/relocs.c
>> index 0c2fae8d929d..4cad603b8d58 100644
>> --- a/arch/x86/tools/relocs.c
>> +++ b/arch/x86/tools/relocs.c
>> @@ -397,7 +397,7 @@ static void read_shdrs(FILE *fp)
>> ehdr.e_shnum);
>> }
>> if (fseek(fp, ehdr.e_shoff, SEEK_SET) < 0) {
>> - die("Seek to %d failed: %s\n",
>> + die("Seek to %"PRIuELF" failed: %s\n",
>> ehdr.e_shoff, strerror(errno));
>> }
>> for (i = 0; i < ehdr.e_shnum; i++) {
>> @@ -431,11 +431,11 @@ static void read_strtabs(FILE *fp)
>> }
>> sec->strtab = malloc(sec->shdr.sh_size);
>> if (!sec->strtab) {
>> - die("malloc of %d bytes for strtab failed\n",
>> + die("malloc of %"PRIuELF" bytes for strtab failed\n",
>> sec->shdr.sh_size);
>> }
>> if (fseek(fp, sec->shdr.sh_offset, SEEK_SET) < 0) {
>> - die("Seek to %d failed: %s\n",
>> + die("Seek to %"PRIuELF" failed: %s\n",
>> sec->shdr.sh_offset, strerror(errno));
>> }
>> if (fread(sec->strtab, 1, sec->shdr.sh_size, fp)
>> @@ -456,11 +456,11 @@ static void read_symtabs(FILE *fp)
&
Re: [PATCH 1/1] drm/i915/dsi: silence a warning about uninitialized return value
Hello,
I sent the patch below a few weeks ago. I got some comments (cf. [1])
which looked good, but the patch has not been merged in linux-next yet.
Do I need to fix something (like rewrite the commit message) in order to
get it merged?
Thanks,
Nicolas
[1] https://patchwork.freedesktop.org/patch/108941/
On 04/09/16 20:58, Nicolas Iooss wrote:
> When building the kernel with clang and some warning flags, the compiler
> reports that the return value of dcs_get_backlight() may be
> uninitialized:
>
> drivers/gpu/drm/i915/intel_dsi_dcs_backlight.c:53:2: error: variable
> 'data' is used uninitialized whenever 'for' loop exits because its
> condition is false [-Werror,-Wsometimes-uninitialized]
> for_each_dsi_port(port, intel_dsi->dcs_backlight_ports) {
> ^~~
> drivers/gpu/drm/i915/intel_dsi.h:126:49: note: expanded from macro
> 'for_each_dsi_port'
> #define for_each_dsi_port(__port, __ports_mask)
> for_each_port_masked(__port, __ports_mask)
> ^~
> drivers/gpu/drm/i915/i915_drv.h:322:26: note: expanded from macro
> 'for_each_port_masked'
> for ((__port) = PORT_A; (__port) < I915_MAX_PORTS; (__port)++) \
> ^
> drivers/gpu/drm/i915/intel_dsi_dcs_backlight.c:60:9: note:
> uninitialized use occurs here
> return data;
>^~~~
>
> As intel_dsi->dcs_backlight_ports seems to be always initialized to a
> non-null value, the content of the for loop is always executed and there
> is no bug in the current code. Nevertheless the compiler has no way of
> knowing that assumption, so initialize variable 'data' to silence the
> warning here.
>
> Signed-off-by: Nicolas Iooss
> ---
> drivers/gpu/drm/i915/intel_dsi_dcs_backlight.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dsi_dcs_backlight.c
> b/drivers/gpu/drm/i915/intel_dsi_dcs_backlight.c
> index ac7c6020c443..eec45856f910 100644
> --- a/drivers/gpu/drm/i915/intel_dsi_dcs_backlight.c
> +++ b/drivers/gpu/drm/i915/intel_dsi_dcs_backlight.c
> @@ -46,7 +46,7 @@ static u32 dcs_get_backlight(struct intel_connector
> *connector)
> struct intel_encoder *encoder = connector->encoder;
> struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
> struct mipi_dsi_device *dsi_device;
> - u8 data;
> + u8 data = 0;
> enum port port;
>
> /* FIXME: Need to take care of 16 bit brightness level */
>
Re: [PATCH 1/1] [media] ite-cir: initialize use_demodulator before using it
Hello,
I sent the following patch (available on
https://patchwork.kernel.org/patch/9325039/) a few weeks ago and got no
feedback even though the bug it fixes seems to still exist in
linux-next. Did I do something wrong, like sending to the wrong maintainers?
Thanks,
Nicolas
On 10/09/16 18:59, Nicolas Iooss wrote:
> Function ite_set_carrier_params() uses variable use_demodulator after
> having initialized it to false in some if branches, but this variable is
> never set to true otherwise.
>
> This bug has been found using clang -Wsometimes-uninitialized warning
> flag.
>
> Fixes: 620a32bba4a2 ("[media] rc: New rc-based ite-cir driver for
> several ITE CIRs")
> Signed-off-by: Nicolas Iooss
> ---
> drivers/media/rc/ite-cir.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/media/rc/ite-cir.c b/drivers/media/rc/ite-cir.c
> index 0f301903aa6f..63165d324fff 100644
> --- a/drivers/media/rc/ite-cir.c
> +++ b/drivers/media/rc/ite-cir.c
> @@ -263,6 +263,8 @@ static void ite_set_carrier_params(struct ite_dev *dev)
>
> if (allowance > ITE_RXDCR_MAX)
> allowance = ITE_RXDCR_MAX;
> +
> + use_demodulator = true;
> }
> }
>
>
Re: [PATCH 1/1] [media] mb86a20s: always initialize a return value
Hello,
I sent the following patch (available on
https://patchwork.kernel.org/patch/9325035/) a few weeks ago and got no
feedback even though the bug it fixes seems to still exist in
linux-next. Did I do something wrong? Should I consider this patch to be
rejected?
Thanks,
Nicolas
On 10/09/16 18:49, Nicolas Iooss wrote:
> In mb86a20s_read_status_and_stats(), when mb86a20s_read_status() fails,
> the function returns the value in variable rc without initializing it
> first. Fix this by propagating the error code from variable status_nr.
>
> This bug has been found using clang and -Wsometimes-uninitialized
> warning flag.
>
> Signed-off-by: Nicolas Iooss
> ---
> drivers/media/dvb-frontends/mb86a20s.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/media/dvb-frontends/mb86a20s.c
> b/drivers/media/dvb-frontends/mb86a20s.c
> index 41325328a22e..eca07432645e 100644
> --- a/drivers/media/dvb-frontends/mb86a20s.c
> +++ b/drivers/media/dvb-frontends/mb86a20s.c
> @@ -1971,6 +1971,7 @@ static int mb86a20s_read_status_and_stats(struct
> dvb_frontend *fe,
> if (status_nr < 0) {
> dev_err(&state->i2c->dev,
> "%s: Can't read frontend lock status\n", __func__);
> + rc = status_nr;
> goto error;
> }
>
>
[PATCH 1/1] libnvdimm, namespace: fix the type of name variable
In create_namespace_blk(), the local variable "name" is defined as an array of NSLABEL_NAME_LEN pointers: char *name[NSLABEL_NAME_LEN]; This variable is then used in calls to memcpy() and kmemdup() as if it were char[NSLABEL_NAME_LEN]. Remove the star in the variable definition to makes it look right. Signed-off-by: Nicolas Iooss --- drivers/nvdimm/namespace_devs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c index abe5c6bc756c..7569ba70cdde 100644 --- a/drivers/nvdimm/namespace_devs.c +++ b/drivers/nvdimm/namespace_devs.c @@ -1997,7 +1997,7 @@ struct device *create_namespace_blk(struct nd_region *nd_region, struct nd_mapping *nd_mapping = &nd_region->mapping[0]; struct nvdimm_drvdata *ndd = to_ndd(nd_mapping); struct nd_namespace_blk *nsblk; - char *name[NSLABEL_NAME_LEN]; + char name[NSLABEL_NAME_LEN]; struct device *dev = NULL; struct resource *res; -- 2.10.2
[PATCH 1/1] kthread: add __printf attributes
When commit fbae2d44aa1d ("kthread: add kthread_create_worker*()")
introduced some kthread_create_...() functions which were taking
printf-like parametter, it introduced __printf attributes to some
functions (e.g. kthread_create_worker()). Nevertheless some new
functions were forgotten (they have been detected thanks to
-Wmissing-format-attribute warning flag).
Add the missing __printf attributes to the newly-introduced functions in
order to detect formatting issues at build-time with -Wformat flag.
Signed-off-by: Nicolas Iooss
---
include/linux/kthread.h | 2 +-
kernel/kthread.c| 5 +++--
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/include/linux/kthread.h b/include/linux/kthread.h
index a6e82a69c363..4de93a54f20b 100644
--- a/include/linux/kthread.h
+++ b/include/linux/kthread.h
@@ -174,7 +174,7 @@ __printf(2, 3)
struct kthread_worker *
kthread_create_worker(unsigned int flags, const char namefmt[], ...);
-struct kthread_worker *
+__printf(3, 4) struct kthread_worker *
kthread_create_worker_on_cpu(int cpu, unsigned int flags,
const char namefmt[], ...);
diff --git a/kernel/kthread.c b/kernel/kthread.c
index be2cc1f9dd57..2baf9354c5fc 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -244,7 +244,8 @@ static void create_kthread(struct kthread_create_info
*create)
}
}
-static struct task_struct *__kthread_create_on_node(int (*threadfn)(void
*data),
+static __printf(4, 0)
+struct task_struct *__kthread_create_on_node(int (*threadfn)(void *data),
void *data, int node,
const char namefmt[],
va_list args)
@@ -630,7 +631,7 @@ int kthread_worker_fn(void *worker_ptr)
}
EXPORT_SYMBOL_GPL(kthread_worker_fn);
-static struct kthread_worker *
+static __printf(3, 0) struct kthread_worker *
__kthread_create_worker(int cpu, unsigned int flags,
const char namefmt[], va_list args)
{
--
2.10.2
[PATCH 1/1] [media] tw686x: silent -Wformat-security warning
Using sprintf() with a non-literal string makes some compiler complain
when building with -Wformat-security (eg. clang reports "format string
is not a string literal (potentially insecure)").
Here sprintf() format parameter is indirectly a literal string so there
is no security issue. Nevertheless adding a "%s" format string to
silent the warning helps to detect real bugs in the kernel.
Signed-off-by: Nicolas Iooss
---
drivers/media/pci/tw686x/tw686x-core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/pci/tw686x/tw686x-core.c
b/drivers/media/pci/tw686x/tw686x-core.c
index 71a0453b1af1..336e2f9bc1b6 100644
--- a/drivers/media/pci/tw686x/tw686x-core.c
+++ b/drivers/media/pci/tw686x/tw686x-core.c
@@ -74,7 +74,7 @@ static const char *dma_mode_name(unsigned int mode)
static int tw686x_dma_mode_get(char *buffer, struct kernel_param *kp)
{
- return sprintf(buffer, dma_mode_name(dma_mode));
+ return sprintf(buffer, "%s", dma_mode_name(dma_mode));
}
static int tw686x_dma_mode_set(const char *val, struct kernel_param *kp)
--
2.10.2
Re: [PATCH 1/1] nvdimm: use the right length of "pmem"
On 30/10/16 16:50, Dan Williams wrote:
> On Sat, Oct 29, 2016 at 4:28 AM, Nicolas Iooss
> wrote:
>> In order to test that the name of a resource begins with "pmem", call
>> strncmp() with 4 as length instead of 3 to match the whole prefix.
>>
>> Fixes: 16660eaea0cc ("libnvdimm, namespace: update label implementation
>> for multi-pmem")
>> Signed-off-by: Nicolas Iooss
>
> Thanks, although I would not call this out as a fix since the length
> parameter could be 1 and still do the right thing. I.e. we're
> distinguishing "blk" from "pmem" resources. I'll add this for 4.10 as
> a cleanup.
All right. Thanks for your quick reply!
Nicolas
[PATCH 2/2] HID: intel-ish-hid: format 32-bit integers with %X
In ishtp_hid_probe(), use %04X instead of %04hX to format __u32 values,
in order to silent a format error reported by clang:
drivers/hid/intel-ish-hid/ishtp-hid.c:212:3: error: format specifies
type 'unsigned short' but the argument has type '__u32' (aka
'unsigned int') [-Werror,-Wformat]
hid->vendor, hid->product);
^~~
drivers/hid/intel-ish-hid/ishtp-hid.c:212:16: error: format
specifies type 'unsigned short' but the argument has type '__u32'
(aka 'unsigned int') [-Werror,-Wformat]
hid->vendor, hid->product);
^~~~
Signed-off-by: Nicolas Iooss
---
drivers/hid/intel-ish-hid/ishtp-hid.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hid/intel-ish-hid/ishtp-hid.c
b/drivers/hid/intel-ish-hid/ishtp-hid.c
index 277983aa1d90..cd23903ddcf1 100644
--- a/drivers/hid/intel-ish-hid/ishtp-hid.c
+++ b/drivers/hid/intel-ish-hid/ishtp-hid.c
@@ -208,7 +208,7 @@ int ishtp_hid_probe(unsigned int cur_hid_dev,
hid->version = le16_to_cpu(ISH_HID_VERSION);
hid->vendor = le16_to_cpu(ISH_HID_VENDOR);
hid->product = le16_to_cpu(ISH_HID_PRODUCT);
- snprintf(hid->name, sizeof(hid->name), "%s %04hX:%04hX", "hid-ishtp",
+ snprintf(hid->name, sizeof(hid->name), "%s %04X:%04X", "hid-ishtp",
hid->vendor, hid->product);
rv = hid_add_device(hid);
--
2.11.0
