Some cosmetic fixes to the patch "UBI: Extend UBI layer debug/messaging
capabilities".
Signed-off-by: Tanya Brokhman <[email protected]>
---
Changes from original patch:
- Added ptr verification @ ubi_err/ubi_msg/ubi_warn
Removed extra printing of ubi number
Removed new messages.
drivers/mtd/ubi/build.c | 6 +++---
drivers/mtd/ubi/cdev.c | 9 ++++-----
drivers/mtd/ubi/io.c | 3 +--
drivers/mtd/ubi/ubi.h | 9 ++++++---
drivers/mtd/ubi/vtbl.c | 7 +++----
drivers/mtd/ubi/wl.c | 10 ++--------
6 files changed, 19 insertions(+), 25 deletions(-)
diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
index 3405be4..ba01a8d 100644
--- a/drivers/mtd/ubi/build.c
+++ b/drivers/mtd/ubi/build.c
@@ -923,7 +923,7 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num,
/* Make sure ubi_num is not busy */
if (ubi_devices[ubi_num]) {
- ubi_err(ubi, "ubi%d already exists", ubi_num);
+ ubi_err(ubi, "already exists");
return -EEXIST;
}
}
@@ -973,7 +973,7 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num,
mutex_init(&ubi->fm_mutex);
init_rwsem(&ubi->fm_sem);
- ubi_msg(ubi, "attaching mtd%d to ubi%d", mtd->index, ubi_num);
+ ubi_msg(ubi, "attaching mtd%d", mtd->index);
err = io_init(ubi, max_beb_per1024);
if (err)
@@ -1428,7 +1428,7 @@ static int __init ubi_mtd_param_parse(const char *val, struct kernel_param *kp)
}
if (len == 0) {
- pr_err("UBI warning: empty 'mtd=' parameter - ignored\n");
+ pr_warn("UBI warning: empty 'mtd=' parameter - ignored\n");
return 0;
}
diff --git a/drivers/mtd/ubi/cdev.c b/drivers/mtd/ubi/cdev.c
index 3410ea81..bbef168 100644
--- a/drivers/mtd/ubi/cdev.c
+++ b/drivers/mtd/ubi/cdev.c
@@ -48,14 +48,13 @@
/**
* get_exclusive - get exclusive access to an UBI volume.
- * @ubi: UBI device description object
* @desc: volume descriptor
*
* This function changes UBI volume open mode to "exclusive". Returns previous
* mode value (positive integer) in case of success and a negative error code
* in case of failure.
*/
-static int get_exclusive(struct ubi_device *ubi, struct ubi_volume_desc *desc)
+static int get_exclusive(struct ubi_volume_desc *desc)
{
int users, err;
struct ubi_volume *vol = desc->vol;
@@ -64,7 +63,7 @@ static int get_exclusive(struct ubi_device *ubi, struct ubi_volume_desc *desc)
users = vol->readers + vol->writers + vol->exclusive;
ubi_assert(users > 0);
if (users > 1) {
- ubi_err(ubi, "%d users for volume %d", users, vol->vol_id);
+ ubi_err(vol->ubi, "%d users for volume %d", users, vol->vol_id);
err = -EBUSY;
} else {
vol->readers = vol->writers = 0;
@@ -421,7 +420,7 @@ static long vol_cdev_ioctl(struct file *file, unsigned int cmd,
break;
}
- err = get_exclusive(ubi, desc);
+ err = get_exclusive(desc);
if (err < 0)
break;
@@ -457,7 +456,7 @@ static long vol_cdev_ioctl(struct file *file, unsigned int cmd,
req.bytes < 0 || req.lnum >= vol->usable_leb_size)
break;
- err = get_exclusive(ubi, desc);
+ err = get_exclusive(desc);
if (err < 0)
break;
diff --git a/drivers/mtd/ubi/io.c b/drivers/mtd/ubi/io.c
index 396aaa5..ed0bcb3 100644
--- a/drivers/mtd/ubi/io.c
+++ b/drivers/mtd/ubi/io.c
@@ -1419,8 +1419,7 @@ int ubi_self_check_all_ff(struct ubi_device *ubi, int pnum, int offset, int len)
fail:
ubi_err(ubi, "self-check failed for PEB %d", pnum);
- ubi_msg(ubi, "hex dump of the %d-%d region",
- offset, offset + len);
+ ubi_msg(ubi, "hex dump of the %d-%d region", offset, offset + len);
print_hex_dump(KERN_DEBUG, "", DUMP_PREFIX_OFFSET, 32, 1, buf, len, 1);
err = -EINVAL;
error:
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index f80ffab..88c2e9f 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -51,13 +51,16 @@
/* Normal UBI messages */
#define ubi_msg(ubi, fmt, ...) pr_notice("UBI-%d: %s:" fmt "\n", \
- ubi->ubi_num, __func__, ##__VA_ARGS__)
+ (ubi ? ubi->ubi_num : UBI_MAX_DEVICES), \
+ __func__, ##__VA_ARGS__)
/* UBI warning messages */
#define ubi_warn(ubi, fmt, ...) pr_warn("UBI-%d warning: %s: " fmt "\n", \
- ubi->ubi_num, __func__, ##__VA_ARGS__)
+ (ubi ? ubi->ubi_num : UBI_MAX_DEVICES), \
+ __func__, ##__VA_ARGS__)
/* UBI error messages */
#define ubi_err(ubi, fmt, ...) pr_err("UBI-%d error: %s: " fmt "\n", \
- ubi->ubi_num, __func__, ##__VA_ARGS__)
+ (ubi ? ubi->ubi_num : UBI_MAX_DEVICES), \
+ __func__, ##__VA_ARGS__)
/* Background thread name pattern */
#define UBI_BGT_NAME_PATTERN "ubi_bgt%dd"
diff --git a/drivers/mtd/ubi/vtbl.c b/drivers/mtd/ubi/vtbl.c
index f8fc308..68c9c5ea 100644
--- a/drivers/mtd/ubi/vtbl.c
+++ b/drivers/mtd/ubi/vtbl.c
@@ -655,14 +655,13 @@ static int init_volumes(struct ubi_device *ubi,
/**
* check_av - check volume attaching information.
- * @ubi: UBI device description object
* @vol: UBI volume description object
* @av: volume attaching information
*
* This function returns zero if the volume attaching information is consistent
* to the data read from the volume tabla, and %-EINVAL if not.
*/
-static int check_av(const struct ubi_device *ubi, const struct ubi_volume *vol,
+static int check_av(const struct ubi_volume *vol,
const struct ubi_ainf_volume *av)
{
int err;
@@ -690,7 +689,7 @@ static int check_av(const struct ubi_device *ubi, const struct ubi_volume *vol,
return 0;
bad:
- ubi_err(ubi, "bad attaching information, error %d", err);
+ ubi_err(vol->ubi, "bad attaching information, error %d", err);
ubi_dump_av(av);
ubi_dump_vol_info(vol);
return -EINVAL;
@@ -753,7 +752,7 @@ static int check_attaching_info(const struct ubi_device *ubi,
ubi_msg(ubi, "finish volume %d removal", av->vol_id);
ubi_remove_av(ai, av);
} else if (av) {
- err = check_av(ubi, vol, av);
+ err = check_av(vol, av);
if (err)
return err;
}
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index 834f6fe..8f7bde6 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -470,11 +470,8 @@ struct ubi_wl_entry *ubi_wl_get_fm_peb(struct ubi_device *ubi, int anchor)
{
struct ubi_wl_entry *e = NULL;
- if (!ubi->free.rb_node || (ubi->free_count - ubi->beb_rsvd_pebs < 1)) {
- ubi_warn(ubi, "Can't get peb for fastmap:anchor=%d, free_cnt=%d, reserved=%d",
- anchor, ubi->free_count, ubi->beb_rsvd_pebs);
+ if (!ubi->free.rb_node || (ubi->free_count - ubi->beb_rsvd_pebs < 1))
goto out;
- }
if (anchor)
e = find_anchor_wl_entry(&ubi->free);
@@ -1806,11 +1803,8 @@ int ubi_thread(void *u)
for (;;) {
int err;
- if (kthread_should_stop()) {
- ubi_msg(ubi, "background thread \"%s\" should stop, PID %d",
- ubi->bgt_name, task_pid_nr(current));
+ if (kthread_should_stop())
break;
- }
if (try_to_freeze())
continue;
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
On Sun, 2014-11-09 at 13:06 +0200, Tanya Brokhman wrote:
>
> /* Normal UBI messages */
> #define ubi_msg(ubi, fmt, ...) pr_notice("UBI-%d: %s:" fmt "\n", \
> - ubi->ubi_num, __func__, ##__VA_ARGS__)
> + (ubi ? ubi->ubi_num : UBI_MAX_DEVICES), \
> + __func__, ##__VA_ARGS__)
> /* UBI warning messages */
> #define ubi_warn(ubi, fmt, ...) pr_warn("UBI-%d warning: %s: " fmt "\n", \
> - ubi->ubi_num, __func__, ##__VA_ARGS__)
> + (ubi ? ubi->ubi_num : UBI_MAX_DEVICES), \
> + __func__, ##__VA_ARGS__)
> /* UBI error messages */
> #define ubi_err(ubi, fmt, ...) pr_err("UBI-%d error: %s: " fmt "\n", \
> - ubi->ubi_num, __func__, ##__VA_ARGS__)
> + (ubi ? ubi->ubi_num : UBI_MAX_DEVICES), \
> + __func__, ##__VA_ARGS__)
Why did you make these changes? It is preferable to not add another 'if'
statement to this macro to handle one or 2 cases - much bloat, little
gain.
Could we please avoid this?
>
> - if (!ubi->free.rb_node || (ubi->free_count - ubi->beb_rsvd_pebs < 1)) {
> - ubi_warn(ubi, "Can't get peb for fastmap:anchor=%d, free_cnt=%d, reserved=%d",
> - anchor, ubi->free_count, ubi->beb_rsvd_pebs);
> + if (!ubi->free.rb_node || (ubi->free_count - ubi->beb_rsvd_pebs < 1))
> goto out;
The warning looks pretty poor, so I do not mind to remove it, but I
thought your patch is about adding a parameter, but you mix different
kinds of things there. Please, be stricter to the similar UBIFS patch
which you was going to send.
> - if (kthread_should_stop()) {
> - ubi_msg(ubi, "background thread \"%s\" should stop, PID %d",
> - ubi->bgt_name, task_pid_nr(current));
> + if (kthread_should_stop())
> break;
> - }
How about just turning this into a debug message, not removing?
Artem.
On 11/10/2014 2:18 PM, Artem Bityutskiy wrote:
> On Sun, 2014-11-09 at 13:06 +0200, Tanya Brokhman wrote:
>>
>> /* Normal UBI messages */
>> #define ubi_msg(ubi, fmt, ...) pr_notice("UBI-%d: %s:" fmt "\n", \
>> - ubi->ubi_num, __func__, ##__VA_ARGS__)
>> + (ubi ? ubi->ubi_num : UBI_MAX_DEVICES), \
>> + __func__, ##__VA_ARGS__)
>> /* UBI warning messages */
>> #define ubi_warn(ubi, fmt, ...) pr_warn("UBI-%d warning: %s: " fmt "\n", \
>> - ubi->ubi_num, __func__, ##__VA_ARGS__)
>> + (ubi ? ubi->ubi_num : UBI_MAX_DEVICES), \
>> + __func__, ##__VA_ARGS__)
>> /* UBI error messages */
>> #define ubi_err(ubi, fmt, ...) pr_err("UBI-%d error: %s: " fmt "\n", \
>> - ubi->ubi_num, __func__, ##__VA_ARGS__)
>> + (ubi ? ubi->ubi_num : UBI_MAX_DEVICES), \
>> + __func__, ##__VA_ARGS__)
>
> Why did you make these changes? It is preferable to not add another 'if'
> statement to this macro to handle one or 2 cases - much bloat, little
> gain.
>
> Could we please avoid this?
I just wanted to be on the safe side and prevent this macro being called
with ubi=NULL that may crash the system. If you still prefer the "if"
removed will do.
>
>>
>> - if (!ubi->free.rb_node || (ubi->free_count - ubi->beb_rsvd_pebs < 1)) {
>> - ubi_warn(ubi, "Can't get peb for fastmap:anchor=%d, free_cnt=%d, reserved=%d",
>> - anchor, ubi->free_count, ubi->beb_rsvd_pebs);
>> + if (!ubi->free.rb_node || (ubi->free_count - ubi->beb_rsvd_pebs < 1))
>> goto out;
>
> The warning looks pretty poor, so I do not mind to remove it, but I
> thought your patch is about adding a parameter, but you mix different
> kinds of things there. Please, be stricter to the similar UBIFS patch
> which you was going to send.
Now I'm confused. I added this msg as part of the patch you already
pushed to your branch but later you requested NOT to add additional msgs
and if required add it in a different patch. So this was added by me and
now removed by me - as per your request.
>
>
>> - if (kthread_should_stop()) {
>> - ubi_msg(ubi, "background thread \"%s\" should stop, PID %d",
>> - ubi->bgt_name, task_pid_nr(current));
>> + if (kthread_should_stop())
>> break;
>> - }
>
> How about just turning this into a debug message, not removing?
Same here. Removing this because *you* requested it.
Quoting you from V5:
"Yes, please, remove these messages or turn them into debugging messages.
And yes, these should have been added in a separate patch."
>
> Artem.
>
Thanks,
Tanya Brokhman
--
Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
On Mon, 2014-11-10 at 14:53 +0200, Tanya Brokhman wrote:
> On 11/10/2014 2:18 PM, Artem Bityutskiy wrote:
> > On Sun, 2014-11-09 at 13:06 +0200, Tanya Brokhman wrote:
> >>
> >> /* Normal UBI messages */
> >> #define ubi_msg(ubi, fmt, ...) pr_notice("UBI-%d: %s:" fmt "\n", \
> >> - ubi->ubi_num, __func__, ##__VA_ARGS__)
> >> + (ubi ? ubi->ubi_num : UBI_MAX_DEVICES), \
> >> + __func__, ##__VA_ARGS__)
> >> /* UBI warning messages */
> >> #define ubi_warn(ubi, fmt, ...) pr_warn("UBI-%d warning: %s: " fmt "\n", \
> >> - ubi->ubi_num, __func__, ##__VA_ARGS__)
> >> + (ubi ? ubi->ubi_num : UBI_MAX_DEVICES), \
> >> + __func__, ##__VA_ARGS__)
> >> /* UBI error messages */
> >> #define ubi_err(ubi, fmt, ...) pr_err("UBI-%d error: %s: " fmt "\n", \
> >> - ubi->ubi_num, __func__, ##__VA_ARGS__)
> >> + (ubi ? ubi->ubi_num : UBI_MAX_DEVICES), \
> >> + __func__, ##__VA_ARGS__)
> >
> > Why did you make these changes? It is preferable to not add another 'if'
> > statement to this macro to handle one or 2 cases - much bloat, little
> > gain.
> >
> > Could we please avoid this?
>
> I just wanted to be on the safe side and prevent this macro being called
> with ubi=NULL that may crash the system. If you still prefer the "if"
> removed will do.
On the other hand, these are macros, and this if gets duplicated in many
places and translate into few additional assembly instructions per
message.
> > The warning looks pretty poor, so I do not mind to remove it, but I
> > thought your patch is about adding a parameter, but you mix different
> > kinds of things there. Please, be stricter to the similar UBIFS patch
> > which you was going to send.
>
> Now I'm confused. I added this msg as part of the patch you already
> pushed to your branch but later you requested NOT to add additional msgs
> and if required add it in a different patch. So this was added by me and
> now removed by me - as per your request.
This comment of mine just repeats that request. It talks about being
stricter in the future patches and not add/remove messages. It does not
request to modify this patch. IOW, this change is OK, but please, let's
make sure we do not have them in the UBIFS patch.
> > How about just turning this into a debug message, not removing?
>
> Same here. Removing this because *you* requested it.
> Quoting you from V5:
> "Yes, please, remove these messages or turn them into debugging messages.
> And yes, these should have been added in a separate patch."
OK, just asking.
On Mon, Nov 10, 2014 at 1:53 PM, Tanya Brokhman <[email protected]> wrote:
> On 11/10/2014 2:18 PM, Artem Bityutskiy wrote:
>>
>> On Sun, 2014-11-09 at 13:06 +0200, Tanya Brokhman wrote:
>>>
>>>
>>> /* Normal UBI messages */
>>> #define ubi_msg(ubi, fmt, ...) pr_notice("UBI-%d: %s:" fmt "\n", \
>>> - ubi->ubi_num, __func__,
>>> ##__VA_ARGS__)
>>> + (ubi ? ubi->ubi_num : UBI_MAX_DEVICES), \
>>> + __func__, ##__VA_ARGS__)
>>> /* UBI warning messages */
>>> #define ubi_warn(ubi, fmt, ...) pr_warn("UBI-%d warning: %s: " fmt
>>> "\n", \
>>> - ubi->ubi_num, __func__,
>>> ##__VA_ARGS__)
>>> + (ubi ? ubi->ubi_num : UBI_MAX_DEVICES), \
>>> + __func__, ##__VA_ARGS__)
>>> /* UBI error messages */
>>> #define ubi_err(ubi, fmt, ...) pr_err("UBI-%d error: %s: " fmt "\n", \
>>> - ubi->ubi_num, __func__,
>>> ##__VA_ARGS__)
>>> + (ubi ? ubi->ubi_num : UBI_MAX_DEVICES), \
>>> + __func__, ##__VA_ARGS__)
>>
>>
>> Why did you make these changes? It is preferable to not add another 'if'
>> statement to this macro to handle one or 2 cases - much bloat, little
>> gain.
>>
>> Could we please avoid this?
>
>
> I just wanted to be on the safe side and prevent this macro being called
> with ubi=NULL that may crash the system. If you still prefer the "if"
> removed will do.
>
>>
>>>
>>> - if (!ubi->free.rb_node || (ubi->free_count - ubi->beb_rsvd_pebs <
>>> 1)) {
>>> - ubi_warn(ubi, "Can't get peb for fastmap:anchor=%d,
>>> free_cnt=%d, reserved=%d",
>>> - anchor, ubi->free_count, ubi->beb_rsvd_pebs);
>>> + if (!ubi->free.rb_node || (ubi->free_count - ubi->beb_rsvd_pebs <
>>> 1))
>>> goto out;
>>
>>
>> The warning looks pretty poor, so I do not mind to remove it, but I
>> thought your patch is about adding a parameter, but you mix different
>> kinds of things there. Please, be stricter to the similar UBIFS patch
>> which you was going to send.
>
>
> Now I'm confused. I added this msg as part of the patch you already pushed
> to your branch but later you requested NOT to add additional msgs and if
> required add it in a different patch. So this was added by me and now
> removed by me - as per your request.
Why do you need that new warning anyways?
It was added by "UBI: Extend UBI layer debug/messaging capabilities".
>>
>>
>>> - if (kthread_should_stop()) {
>>> - ubi_msg(ubi, "background thread \"%s\" should
>>> stop, PID %d",
>>> - ubi->bgt_name, task_pid_nr(current));
>>> + if (kthread_should_stop())
>>> break;
>>> - }
>>
>>
>> How about just turning this into a debug message, not removing?
>
>
> Same here. Removing this because *you* requested it.
> Quoting you from V5:
> "Yes, please, remove these messages or turn them into debugging messages.
> And yes, these should have been added in a separate patch."
>
>>
>> Artem.
>>
>
>
> Thanks,
> Tanya Brokhman
> --
> Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
Thanks,
//richard
On Mon, 2014-11-10 at 15:14 +0200, Artem Bityutskiy wrote:
> On Mon, 2014-11-10 at 14:53 +0200, Tanya Brokhman wrote:
> > On 11/10/2014 2:18 PM, Artem Bityutskiy wrote:
> > > On Sun, 2014-11-09 at 13:06 +0200, Tanya Brokhman wrote:
> > >>
> > >> /* Normal UBI messages */
> > >> #define ubi_msg(ubi, fmt, ...) pr_notice("UBI-%d: %s:" fmt "\n", \
> > >> - ubi->ubi_num, __func__, ##__VA_ARGS__)
> > >> + (ubi ? ubi->ubi_num : UBI_MAX_DEVICES), \
> > >> + __func__, ##__VA_ARGS__)
> > >> /* UBI warning messages */
> > >> #define ubi_warn(ubi, fmt, ...) pr_warn("UBI-%d warning: %s: " fmt "\n", \
> > >> - ubi->ubi_num, __func__, ##__VA_ARGS__)
> > >> + (ubi ? ubi->ubi_num : UBI_MAX_DEVICES), \
> > >> + __func__, ##__VA_ARGS__)
> > >> /* UBI error messages */
> > >> #define ubi_err(ubi, fmt, ...) pr_err("UBI-%d error: %s: " fmt "\n", \
> > >> - ubi->ubi_num, __func__, ##__VA_ARGS__)
> > >> + (ubi ? ubi->ubi_num : UBI_MAX_DEVICES), \
> > >> + __func__, ##__VA_ARGS__)
> > >
> > > Why did you make these changes? It is preferable to not add another 'if'
> > > statement to this macro to handle one or 2 cases - much bloat, little
> > > gain.
> > >
> > > Could we please avoid this?
> >
> > I just wanted to be on the safe side and prevent this macro being called
> > with ubi=NULL that may crash the system. If you still prefer the "if"
> > removed will do.
>
> On the other hand, these are macros, and this if gets duplicated in many
> places and translate into few additional assembly instructions per
> message.
The thing that will make these uses smaller is to
convert them to functions.
There is a lot of extra duplicated "UBI-%s <msg_type>: "
constant string .text added.
Using a function uses a single copy of each prefix.
The __func__ variable can also be removed.
__builtin_return_address(0) may be substituted to save
a few more bytes per instance.
Something like:
(prototype)
__printf(2, 3)
void ubi_warn(struct ubi *ubi, const char *fmt, ...);
(implementation)
__printf(2, 3)
void ubi_warn(struct ubi *ubi, const char *fmt, ...)
{
struct va_format vaf;
va_list args;
int device;
va_start(args, format);
vaf.fmt = format;
vaf.va = &args;
if (!ubi)
device = UBI_MAX_DEVICE;
else
device = ubi->ubi_num;
pr_warn("UBI-%d warning: %pf: %pV",
device, __builtin_return_address(0), &vaf);
va_end(args);
}
On Sun, 2014-11-09 at 13:06 +0200, Tanya Brokhman wrote:
> Some cosmetic fixes to the patch "UBI: Extend UBI layer debug/messaging
> capabilities".
>
> Signed-off-by: Tanya Brokhman <[email protected]>
> ---
Pushed this patch, but without the hunk which changes the printing
helpers. Thanks!