Re: [RFC 3/6] pstore: remove max argument from ramoops_get_next_prz
On Fri, Oct 26, 2018 at 08:27:49PM +0100, Kees Cook wrote:
> On Fri, Oct 26, 2018 at 8:22 PM, Joel Fernandes
> wrote:
> > On Fri, Oct 26, 2018 at 11:00:39AM -0700, Joel Fernandes (Google) wrote:
> >> From the code flow, the 'max' checks are already being done on the prz
> >> passed to ramoops_get_next_prz. Lets remove it to simplify this function
> >> and reduce its arguments.
> >>
> >> Signed-off-by: Joel Fernandes (Google)
> >> ---
> >> fs/pstore/ram.c | 14 ++
> >> 1 file changed, 6 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> >> index cbfdf4b8e89d..3055e05acab1 100644
> >> --- a/fs/pstore/ram.c
> >> +++ b/fs/pstore/ram.c
> >> @@ -124,14 +124,14 @@ static int ramoops_pstore_open(struct pstore_info
> >> *psi)
> >> }
> >>
> >> static struct persistent_ram_zone *
> >> -ramoops_get_next_prz(struct persistent_ram_zone *przs[], uint *c, uint
> >> max,
> >> +ramoops_get_next_prz(struct persistent_ram_zone *przs[], uint *c,
> >>u64 *id, enum pstore_type_id *typep, bool update)
> >> {
> >> struct persistent_ram_zone *prz;
> >> int i = (*c)++;
> >>
> >> /* Give up if we never existed or have hit the end. */
> >> - if (!przs || i >= max)
> >> + if (!przs)
> >> return NULL;
> >>
> >> prz = przs[i];
> >
> > Ah, looks like I may have introduced an issue here since 'i' isn't checked
> > by
> > the caller for the single prz case, its only checked for the multiple prz
> > cases, so something like below could be folded in. I still feel its better
> > than passing the max argument.
> >
> > Another thought is, even better we could have a different function when
> > there's only one prz and not have to pass an array, just pass the first
> > element? Something like...
> >
> > ramoops_get_next_prz_single(struct persistent_ram_zone *prz, uint *c,
> > enum pstore_type_id *typep, bool update)
> > And for the _single case, we also wouldn't need to pass id so that's
> > another
> > argument less.
> >
> > Let me know what you think, otherwise something like the below will need to
> > be folded in to fix this patch... thanks.
> >
> > 8<---
> >
> > diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> > index 5702b692bdb9..061d2af2485b 100644
> > --- a/fs/pstore/ram.c
> > +++ b/fs/pstore/ram.c
> > @@ -268,17 +268,19 @@ static ssize_t ramoops_pstore_read(struct
> > pstore_record *record)
> > }
> > }
> >
> > - if (!prz_ok(prz))
> > + if (!prz_ok(prz) && !cxt->console_read_cnt) {
> > prz = ramoops_get_next_prz(>cprz,
> > >console_read_cnt,
> >record, 0);
> > + }
> >
> > - if (!prz_ok(prz))
> > + if (!prz_ok(prz) && !cxt->pmsg_read_cnt)
> > prz = ramoops_get_next_prz(>mprz, >pmsg_read_cnt,
> >record, 0);
> >
> > /* ftrace is last since it may want to dynamically allocate memory.
> > */
> > if (!prz_ok(prz)) {
> > - if (!(cxt->flags & RAMOOPS_FLAG_FTRACE_PER_CPU)) {
> > + if (!(cxt->flags & RAMOOPS_FLAG_FTRACE_PER_CPU) &&
> > + !cxt->ftrace_read_cnt) {
> > prz = ramoops_get_next_prz(cxt->fprzs,
> > >ftrace_read_cnt, record, 0);
> > } else {
>
> Ah yeah, good catch! I think your added fix is right. I was pondering
> asking you to remove the & on the *_read_cnt and having the caller do
> the increment:
>
> while (cxt->dump_read_cnt < cxt->max_dump_cnt && !prz) {
> prz = ramoops_get_next_prz(cxt->dprzs, cxt->dump_read_cnt++,
>>id,
>>type,
>PSTORE_TYPE_DMESG, 1);
Sure, that's better, I'll do that. That we don't have to pass a pointer, the
caller knows about the increment, and its a local variable less. thanks!
- Joel
Re: [RFC 3/6] pstore: remove max argument from ramoops_get_next_prz
On Fri, Oct 26, 2018 at 08:27:49PM +0100, Kees Cook wrote:
> On Fri, Oct 26, 2018 at 8:22 PM, Joel Fernandes
> wrote:
> > On Fri, Oct 26, 2018 at 11:00:39AM -0700, Joel Fernandes (Google) wrote:
> >> From the code flow, the 'max' checks are already being done on the prz
> >> passed to ramoops_get_next_prz. Lets remove it to simplify this function
> >> and reduce its arguments.
> >>
> >> Signed-off-by: Joel Fernandes (Google)
> >> ---
> >> fs/pstore/ram.c | 14 ++
> >> 1 file changed, 6 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> >> index cbfdf4b8e89d..3055e05acab1 100644
> >> --- a/fs/pstore/ram.c
> >> +++ b/fs/pstore/ram.c
> >> @@ -124,14 +124,14 @@ static int ramoops_pstore_open(struct pstore_info
> >> *psi)
> >> }
> >>
> >> static struct persistent_ram_zone *
> >> -ramoops_get_next_prz(struct persistent_ram_zone *przs[], uint *c, uint
> >> max,
> >> +ramoops_get_next_prz(struct persistent_ram_zone *przs[], uint *c,
> >>u64 *id, enum pstore_type_id *typep, bool update)
> >> {
> >> struct persistent_ram_zone *prz;
> >> int i = (*c)++;
> >>
> >> /* Give up if we never existed or have hit the end. */
> >> - if (!przs || i >= max)
> >> + if (!przs)
> >> return NULL;
> >>
> >> prz = przs[i];
> >
> > Ah, looks like I may have introduced an issue here since 'i' isn't checked
> > by
> > the caller for the single prz case, its only checked for the multiple prz
> > cases, so something like below could be folded in. I still feel its better
> > than passing the max argument.
> >
> > Another thought is, even better we could have a different function when
> > there's only one prz and not have to pass an array, just pass the first
> > element? Something like...
> >
> > ramoops_get_next_prz_single(struct persistent_ram_zone *prz, uint *c,
> > enum pstore_type_id *typep, bool update)
> > And for the _single case, we also wouldn't need to pass id so that's
> > another
> > argument less.
> >
> > Let me know what you think, otherwise something like the below will need to
> > be folded in to fix this patch... thanks.
> >
> > 8<---
> >
> > diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> > index 5702b692bdb9..061d2af2485b 100644
> > --- a/fs/pstore/ram.c
> > +++ b/fs/pstore/ram.c
> > @@ -268,17 +268,19 @@ static ssize_t ramoops_pstore_read(struct
> > pstore_record *record)
> > }
> > }
> >
> > - if (!prz_ok(prz))
> > + if (!prz_ok(prz) && !cxt->console_read_cnt) {
> > prz = ramoops_get_next_prz(>cprz,
> > >console_read_cnt,
> >record, 0);
> > + }
> >
> > - if (!prz_ok(prz))
> > + if (!prz_ok(prz) && !cxt->pmsg_read_cnt)
> > prz = ramoops_get_next_prz(>mprz, >pmsg_read_cnt,
> >record, 0);
> >
> > /* ftrace is last since it may want to dynamically allocate memory.
> > */
> > if (!prz_ok(prz)) {
> > - if (!(cxt->flags & RAMOOPS_FLAG_FTRACE_PER_CPU)) {
> > + if (!(cxt->flags & RAMOOPS_FLAG_FTRACE_PER_CPU) &&
> > + !cxt->ftrace_read_cnt) {
> > prz = ramoops_get_next_prz(cxt->fprzs,
> > >ftrace_read_cnt, record, 0);
> > } else {
>
> Ah yeah, good catch! I think your added fix is right. I was pondering
> asking you to remove the & on the *_read_cnt and having the caller do
> the increment:
>
> while (cxt->dump_read_cnt < cxt->max_dump_cnt && !prz) {
> prz = ramoops_get_next_prz(cxt->dprzs, cxt->dump_read_cnt++,
>>id,
>>type,
>PSTORE_TYPE_DMESG, 1);
Sure, that's better, I'll do that. That we don't have to pass a pointer, the
caller knows about the increment, and its a local variable less. thanks!
- Joel
Re: [RFC 3/6] pstore: remove max argument from ramoops_get_next_prz
On Fri, Oct 26, 2018 at 8:22 PM, Joel Fernandes wrote:
> On Fri, Oct 26, 2018 at 11:00:39AM -0700, Joel Fernandes (Google) wrote:
>> From the code flow, the 'max' checks are already being done on the prz
>> passed to ramoops_get_next_prz. Lets remove it to simplify this function
>> and reduce its arguments.
>>
>> Signed-off-by: Joel Fernandes (Google)
>> ---
>> fs/pstore/ram.c | 14 ++
>> 1 file changed, 6 insertions(+), 8 deletions(-)
>>
>> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
>> index cbfdf4b8e89d..3055e05acab1 100644
>> --- a/fs/pstore/ram.c
>> +++ b/fs/pstore/ram.c
>> @@ -124,14 +124,14 @@ static int ramoops_pstore_open(struct pstore_info *psi)
>> }
>>
>> static struct persistent_ram_zone *
>> -ramoops_get_next_prz(struct persistent_ram_zone *przs[], uint *c, uint max,
>> +ramoops_get_next_prz(struct persistent_ram_zone *przs[], uint *c,
>>u64 *id, enum pstore_type_id *typep, bool update)
>> {
>> struct persistent_ram_zone *prz;
>> int i = (*c)++;
>>
>> /* Give up if we never existed or have hit the end. */
>> - if (!przs || i >= max)
>> + if (!przs)
>> return NULL;
>>
>> prz = przs[i];
>
> Ah, looks like I may have introduced an issue here since 'i' isn't checked by
> the caller for the single prz case, its only checked for the multiple prz
> cases, so something like below could be folded in. I still feel its better
> than passing the max argument.
>
> Another thought is, even better we could have a different function when
> there's only one prz and not have to pass an array, just pass the first
> element? Something like...
>
> ramoops_get_next_prz_single(struct persistent_ram_zone *prz, uint *c,
> enum pstore_type_id *typep, bool update)
> And for the _single case, we also wouldn't need to pass id so that's another
> argument less.
>
> Let me know what you think, otherwise something like the below will need to
> be folded in to fix this patch... thanks.
>
> 8<---
>
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index 5702b692bdb9..061d2af2485b 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -268,17 +268,19 @@ static ssize_t ramoops_pstore_read(struct pstore_record
> *record)
> }
> }
>
> - if (!prz_ok(prz))
> + if (!prz_ok(prz) && !cxt->console_read_cnt) {
> prz = ramoops_get_next_prz(>cprz, >console_read_cnt,
>record, 0);
> + }
>
> - if (!prz_ok(prz))
> + if (!prz_ok(prz) && !cxt->pmsg_read_cnt)
> prz = ramoops_get_next_prz(>mprz, >pmsg_read_cnt,
>record, 0);
>
> /* ftrace is last since it may want to dynamically allocate memory. */
> if (!prz_ok(prz)) {
> - if (!(cxt->flags & RAMOOPS_FLAG_FTRACE_PER_CPU)) {
> + if (!(cxt->flags & RAMOOPS_FLAG_FTRACE_PER_CPU) &&
> + !cxt->ftrace_read_cnt) {
> prz = ramoops_get_next_prz(cxt->fprzs,
> >ftrace_read_cnt, record, 0);
> } else {
Ah yeah, good catch! I think your added fix is right. I was pondering
asking you to remove the & on the *_read_cnt and having the caller do
the increment:
while (cxt->dump_read_cnt < cxt->max_dump_cnt && !prz) {
prz = ramoops_get_next_prz(cxt->dprzs, cxt->dump_read_cnt++,
>id,
>type,
PSTORE_TYPE_DMESG, 1);
-Kees
--
Kees Cook
Re: [RFC 3/6] pstore: remove max argument from ramoops_get_next_prz
On Fri, Oct 26, 2018 at 8:22 PM, Joel Fernandes wrote:
> On Fri, Oct 26, 2018 at 11:00:39AM -0700, Joel Fernandes (Google) wrote:
>> From the code flow, the 'max' checks are already being done on the prz
>> passed to ramoops_get_next_prz. Lets remove it to simplify this function
>> and reduce its arguments.
>>
>> Signed-off-by: Joel Fernandes (Google)
>> ---
>> fs/pstore/ram.c | 14 ++
>> 1 file changed, 6 insertions(+), 8 deletions(-)
>>
>> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
>> index cbfdf4b8e89d..3055e05acab1 100644
>> --- a/fs/pstore/ram.c
>> +++ b/fs/pstore/ram.c
>> @@ -124,14 +124,14 @@ static int ramoops_pstore_open(struct pstore_info *psi)
>> }
>>
>> static struct persistent_ram_zone *
>> -ramoops_get_next_prz(struct persistent_ram_zone *przs[], uint *c, uint max,
>> +ramoops_get_next_prz(struct persistent_ram_zone *przs[], uint *c,
>>u64 *id, enum pstore_type_id *typep, bool update)
>> {
>> struct persistent_ram_zone *prz;
>> int i = (*c)++;
>>
>> /* Give up if we never existed or have hit the end. */
>> - if (!przs || i >= max)
>> + if (!przs)
>> return NULL;
>>
>> prz = przs[i];
>
> Ah, looks like I may have introduced an issue here since 'i' isn't checked by
> the caller for the single prz case, its only checked for the multiple prz
> cases, so something like below could be folded in. I still feel its better
> than passing the max argument.
>
> Another thought is, even better we could have a different function when
> there's only one prz and not have to pass an array, just pass the first
> element? Something like...
>
> ramoops_get_next_prz_single(struct persistent_ram_zone *prz, uint *c,
> enum pstore_type_id *typep, bool update)
> And for the _single case, we also wouldn't need to pass id so that's another
> argument less.
>
> Let me know what you think, otherwise something like the below will need to
> be folded in to fix this patch... thanks.
>
> 8<---
>
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index 5702b692bdb9..061d2af2485b 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -268,17 +268,19 @@ static ssize_t ramoops_pstore_read(struct pstore_record
> *record)
> }
> }
>
> - if (!prz_ok(prz))
> + if (!prz_ok(prz) && !cxt->console_read_cnt) {
> prz = ramoops_get_next_prz(>cprz, >console_read_cnt,
>record, 0);
> + }
>
> - if (!prz_ok(prz))
> + if (!prz_ok(prz) && !cxt->pmsg_read_cnt)
> prz = ramoops_get_next_prz(>mprz, >pmsg_read_cnt,
>record, 0);
>
> /* ftrace is last since it may want to dynamically allocate memory. */
> if (!prz_ok(prz)) {
> - if (!(cxt->flags & RAMOOPS_FLAG_FTRACE_PER_CPU)) {
> + if (!(cxt->flags & RAMOOPS_FLAG_FTRACE_PER_CPU) &&
> + !cxt->ftrace_read_cnt) {
> prz = ramoops_get_next_prz(cxt->fprzs,
> >ftrace_read_cnt, record, 0);
> } else {
Ah yeah, good catch! I think your added fix is right. I was pondering
asking you to remove the & on the *_read_cnt and having the caller do
the increment:
while (cxt->dump_read_cnt < cxt->max_dump_cnt && !prz) {
prz = ramoops_get_next_prz(cxt->dprzs, cxt->dump_read_cnt++,
>id,
>type,
PSTORE_TYPE_DMESG, 1);
-Kees
--
Kees Cook
Re: [RFC 3/6] pstore: remove max argument from ramoops_get_next_prz
On Fri, Oct 26, 2018 at 7:00 PM, Joel Fernandes (Google)
wrote:
> From the code flow, the 'max' checks are already being done on the prz
> passed to ramoops_get_next_prz. Lets remove it to simplify this function
> and reduce its arguments.
>
> Signed-off-by: Joel Fernandes (Google)
> ---
> fs/pstore/ram.c | 14 ++
> 1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index cbfdf4b8e89d..3055e05acab1 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -124,14 +124,14 @@ static int ramoops_pstore_open(struct pstore_info *psi)
> }
>
> static struct persistent_ram_zone *
> -ramoops_get_next_prz(struct persistent_ram_zone *przs[], uint *c, uint max,
> +ramoops_get_next_prz(struct persistent_ram_zone *przs[], uint *c,
> u64 *id, enum pstore_type_id *typep, bool update)
> {
> struct persistent_ram_zone *prz;
> int i = (*c)++;
>
> /* Give up if we never existed or have hit the end. */
> - if (!przs || i >= max)
> + if (!przs)
> return NULL;
>
> prz = przs[i];
> @@ -254,8 +254,7 @@ static ssize_t ramoops_pstore_read(struct pstore_record
> *record)
> /* Find the next valid persistent_ram_zone for DMESG */
> while (cxt->dump_read_cnt < cxt->max_dump_cnt && !prz) {
> prz = ramoops_get_next_prz(cxt->dprzs, >dump_read_cnt,
> - cxt->max_dump_cnt, >id,
> - >type, 1);
> + >id, >type, 1);
> if (!prz_ok(prz))
> continue;
> header_length = ramoops_read_kmsg_hdr(persistent_ram_old(prz),
> @@ -271,17 +270,17 @@ static ssize_t ramoops_pstore_read(struct pstore_record
> *record)
>
> if (!prz_ok(prz))
> prz = ramoops_get_next_prz(>cprz, >console_read_cnt,
> - 1, >id, >type, 0);
> + >id, >type, 0);
>
> if (!prz_ok(prz))
> prz = ramoops_get_next_prz(>mprz, >pmsg_read_cnt,
> - 1, >id, >type, 0);
> + >id, >type, 0);
>
> /* ftrace is last since it may want to dynamically allocate memory. */
> if (!prz_ok(prz)) {
> if (!(cxt->flags & RAMOOPS_FLAG_FTRACE_PER_CPU)) {
> prz = ramoops_get_next_prz(cxt->fprzs,
> - >ftrace_read_cnt, 1, >id,
> + >ftrace_read_cnt, >id,
> >type, 0);
> } else {
> /*
> @@ -299,7 +298,6 @@ static ssize_t ramoops_pstore_read(struct pstore_record
> *record)
> while (cxt->ftrace_read_cnt < cxt->max_ftrace_cnt) {
> prz_next = ramoops_get_next_prz(cxt->fprzs,
> >ftrace_read_cnt,
> - cxt->max_ftrace_cnt,
> >id,
> >type, 0);
>
> --
> 2.19.1.568.g152ad8e336-goog
>
Yup, looks good.
--
Kees Cook
Re: [RFC 3/6] pstore: remove max argument from ramoops_get_next_prz
On Fri, Oct 26, 2018 at 7:00 PM, Joel Fernandes (Google)
wrote:
> From the code flow, the 'max' checks are already being done on the prz
> passed to ramoops_get_next_prz. Lets remove it to simplify this function
> and reduce its arguments.
>
> Signed-off-by: Joel Fernandes (Google)
> ---
> fs/pstore/ram.c | 14 ++
> 1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index cbfdf4b8e89d..3055e05acab1 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -124,14 +124,14 @@ static int ramoops_pstore_open(struct pstore_info *psi)
> }
>
> static struct persistent_ram_zone *
> -ramoops_get_next_prz(struct persistent_ram_zone *przs[], uint *c, uint max,
> +ramoops_get_next_prz(struct persistent_ram_zone *przs[], uint *c,
> u64 *id, enum pstore_type_id *typep, bool update)
> {
> struct persistent_ram_zone *prz;
> int i = (*c)++;
>
> /* Give up if we never existed or have hit the end. */
> - if (!przs || i >= max)
> + if (!przs)
> return NULL;
>
> prz = przs[i];
> @@ -254,8 +254,7 @@ static ssize_t ramoops_pstore_read(struct pstore_record
> *record)
> /* Find the next valid persistent_ram_zone for DMESG */
> while (cxt->dump_read_cnt < cxt->max_dump_cnt && !prz) {
> prz = ramoops_get_next_prz(cxt->dprzs, >dump_read_cnt,
> - cxt->max_dump_cnt, >id,
> - >type, 1);
> + >id, >type, 1);
> if (!prz_ok(prz))
> continue;
> header_length = ramoops_read_kmsg_hdr(persistent_ram_old(prz),
> @@ -271,17 +270,17 @@ static ssize_t ramoops_pstore_read(struct pstore_record
> *record)
>
> if (!prz_ok(prz))
> prz = ramoops_get_next_prz(>cprz, >console_read_cnt,
> - 1, >id, >type, 0);
> + >id, >type, 0);
>
> if (!prz_ok(prz))
> prz = ramoops_get_next_prz(>mprz, >pmsg_read_cnt,
> - 1, >id, >type, 0);
> + >id, >type, 0);
>
> /* ftrace is last since it may want to dynamically allocate memory. */
> if (!prz_ok(prz)) {
> if (!(cxt->flags & RAMOOPS_FLAG_FTRACE_PER_CPU)) {
> prz = ramoops_get_next_prz(cxt->fprzs,
> - >ftrace_read_cnt, 1, >id,
> + >ftrace_read_cnt, >id,
> >type, 0);
> } else {
> /*
> @@ -299,7 +298,6 @@ static ssize_t ramoops_pstore_read(struct pstore_record
> *record)
> while (cxt->ftrace_read_cnt < cxt->max_ftrace_cnt) {
> prz_next = ramoops_get_next_prz(cxt->fprzs,
> >ftrace_read_cnt,
> - cxt->max_ftrace_cnt,
> >id,
> >type, 0);
>
> --
> 2.19.1.568.g152ad8e336-goog
>
Yup, looks good.
--
Kees Cook
Re: [RFC 3/6] pstore: remove max argument from ramoops_get_next_prz
On Fri, Oct 26, 2018 at 11:00:39AM -0700, Joel Fernandes (Google) wrote:
> From the code flow, the 'max' checks are already being done on the prz
> passed to ramoops_get_next_prz. Lets remove it to simplify this function
> and reduce its arguments.
>
> Signed-off-by: Joel Fernandes (Google)
> ---
> fs/pstore/ram.c | 14 ++
> 1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index cbfdf4b8e89d..3055e05acab1 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -124,14 +124,14 @@ static int ramoops_pstore_open(struct pstore_info *psi)
> }
>
> static struct persistent_ram_zone *
> -ramoops_get_next_prz(struct persistent_ram_zone *przs[], uint *c, uint max,
> +ramoops_get_next_prz(struct persistent_ram_zone *przs[], uint *c,
>u64 *id, enum pstore_type_id *typep, bool update)
> {
> struct persistent_ram_zone *prz;
> int i = (*c)++;
>
> /* Give up if we never existed or have hit the end. */
> - if (!przs || i >= max)
> + if (!przs)
> return NULL;
>
> prz = przs[i];
Ah, looks like I may have introduced an issue here since 'i' isn't checked by
the caller for the single prz case, its only checked for the multiple prz
cases, so something like below could be folded in. I still feel its better
than passing the max argument.
Another thought is, even better we could have a different function when
there's only one prz and not have to pass an array, just pass the first
element? Something like...
ramoops_get_next_prz_single(struct persistent_ram_zone *prz, uint *c,
enum pstore_type_id *typep, bool update)
And for the _single case, we also wouldn't need to pass id so that's another
argument less.
Let me know what you think, otherwise something like the below will need to
be folded in to fix this patch... thanks.
8<---
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 5702b692bdb9..061d2af2485b 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -268,17 +268,19 @@ static ssize_t ramoops_pstore_read(struct pstore_record
*record)
}
}
- if (!prz_ok(prz))
+ if (!prz_ok(prz) && !cxt->console_read_cnt) {
prz = ramoops_get_next_prz(>cprz, >console_read_cnt,
record, 0);
+ }
- if (!prz_ok(prz))
+ if (!prz_ok(prz) && !cxt->pmsg_read_cnt)
prz = ramoops_get_next_prz(>mprz, >pmsg_read_cnt,
record, 0);
/* ftrace is last since it may want to dynamically allocate memory. */
if (!prz_ok(prz)) {
- if (!(cxt->flags & RAMOOPS_FLAG_FTRACE_PER_CPU)) {
+ if (!(cxt->flags & RAMOOPS_FLAG_FTRACE_PER_CPU) &&
+ !cxt->ftrace_read_cnt) {
prz = ramoops_get_next_prz(cxt->fprzs,
>ftrace_read_cnt, record, 0);
} else {
Re: [RFC 3/6] pstore: remove max argument from ramoops_get_next_prz
On Fri, Oct 26, 2018 at 11:00:39AM -0700, Joel Fernandes (Google) wrote:
> From the code flow, the 'max' checks are already being done on the prz
> passed to ramoops_get_next_prz. Lets remove it to simplify this function
> and reduce its arguments.
>
> Signed-off-by: Joel Fernandes (Google)
> ---
> fs/pstore/ram.c | 14 ++
> 1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index cbfdf4b8e89d..3055e05acab1 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -124,14 +124,14 @@ static int ramoops_pstore_open(struct pstore_info *psi)
> }
>
> static struct persistent_ram_zone *
> -ramoops_get_next_prz(struct persistent_ram_zone *przs[], uint *c, uint max,
> +ramoops_get_next_prz(struct persistent_ram_zone *przs[], uint *c,
>u64 *id, enum pstore_type_id *typep, bool update)
> {
> struct persistent_ram_zone *prz;
> int i = (*c)++;
>
> /* Give up if we never existed or have hit the end. */
> - if (!przs || i >= max)
> + if (!przs)
> return NULL;
>
> prz = przs[i];
Ah, looks like I may have introduced an issue here since 'i' isn't checked by
the caller for the single prz case, its only checked for the multiple prz
cases, so something like below could be folded in. I still feel its better
than passing the max argument.
Another thought is, even better we could have a different function when
there's only one prz and not have to pass an array, just pass the first
element? Something like...
ramoops_get_next_prz_single(struct persistent_ram_zone *prz, uint *c,
enum pstore_type_id *typep, bool update)
And for the _single case, we also wouldn't need to pass id so that's another
argument less.
Let me know what you think, otherwise something like the below will need to
be folded in to fix this patch... thanks.
8<---
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 5702b692bdb9..061d2af2485b 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -268,17 +268,19 @@ static ssize_t ramoops_pstore_read(struct pstore_record
*record)
}
}
- if (!prz_ok(prz))
+ if (!prz_ok(prz) && !cxt->console_read_cnt) {
prz = ramoops_get_next_prz(>cprz, >console_read_cnt,
record, 0);
+ }
- if (!prz_ok(prz))
+ if (!prz_ok(prz) && !cxt->pmsg_read_cnt)
prz = ramoops_get_next_prz(>mprz, >pmsg_read_cnt,
record, 0);
/* ftrace is last since it may want to dynamically allocate memory. */
if (!prz_ok(prz)) {
- if (!(cxt->flags & RAMOOPS_FLAG_FTRACE_PER_CPU)) {
+ if (!(cxt->flags & RAMOOPS_FLAG_FTRACE_PER_CPU) &&
+ !cxt->ftrace_read_cnt) {
prz = ramoops_get_next_prz(cxt->fprzs,
>ftrace_read_cnt, record, 0);
} else {
[RFC 3/6] pstore: remove max argument from ramoops_get_next_prz
>From the code flow, the 'max' checks are already being done on the prz
passed to ramoops_get_next_prz. Lets remove it to simplify this function
and reduce its arguments.
Signed-off-by: Joel Fernandes (Google)
---
fs/pstore/ram.c | 14 ++
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index cbfdf4b8e89d..3055e05acab1 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -124,14 +124,14 @@ static int ramoops_pstore_open(struct pstore_info *psi)
}
static struct persistent_ram_zone *
-ramoops_get_next_prz(struct persistent_ram_zone *przs[], uint *c, uint max,
+ramoops_get_next_prz(struct persistent_ram_zone *przs[], uint *c,
u64 *id, enum pstore_type_id *typep, bool update)
{
struct persistent_ram_zone *prz;
int i = (*c)++;
/* Give up if we never existed or have hit the end. */
- if (!przs || i >= max)
+ if (!przs)
return NULL;
prz = przs[i];
@@ -254,8 +254,7 @@ static ssize_t ramoops_pstore_read(struct pstore_record
*record)
/* Find the next valid persistent_ram_zone for DMESG */
while (cxt->dump_read_cnt < cxt->max_dump_cnt && !prz) {
prz = ramoops_get_next_prz(cxt->dprzs, >dump_read_cnt,
- cxt->max_dump_cnt, >id,
- >type, 1);
+ >id, >type, 1);
if (!prz_ok(prz))
continue;
header_length = ramoops_read_kmsg_hdr(persistent_ram_old(prz),
@@ -271,17 +270,17 @@ static ssize_t ramoops_pstore_read(struct pstore_record
*record)
if (!prz_ok(prz))
prz = ramoops_get_next_prz(>cprz, >console_read_cnt,
- 1, >id, >type, 0);
+ >id, >type, 0);
if (!prz_ok(prz))
prz = ramoops_get_next_prz(>mprz, >pmsg_read_cnt,
- 1, >id, >type, 0);
+ >id, >type, 0);
/* ftrace is last since it may want to dynamically allocate memory. */
if (!prz_ok(prz)) {
if (!(cxt->flags & RAMOOPS_FLAG_FTRACE_PER_CPU)) {
prz = ramoops_get_next_prz(cxt->fprzs,
- >ftrace_read_cnt, 1, >id,
+ >ftrace_read_cnt, >id,
>type, 0);
} else {
/*
@@ -299,7 +298,6 @@ static ssize_t ramoops_pstore_read(struct pstore_record
*record)
while (cxt->ftrace_read_cnt < cxt->max_ftrace_cnt) {
prz_next = ramoops_get_next_prz(cxt->fprzs,
>ftrace_read_cnt,
- cxt->max_ftrace_cnt,
>id,
>type, 0);
--
2.19.1.568.g152ad8e336-goog
[RFC 3/6] pstore: remove max argument from ramoops_get_next_prz
>From the code flow, the 'max' checks are already being done on the prz
passed to ramoops_get_next_prz. Lets remove it to simplify this function
and reduce its arguments.
Signed-off-by: Joel Fernandes (Google)
---
fs/pstore/ram.c | 14 ++
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index cbfdf4b8e89d..3055e05acab1 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -124,14 +124,14 @@ static int ramoops_pstore_open(struct pstore_info *psi)
}
static struct persistent_ram_zone *
-ramoops_get_next_prz(struct persistent_ram_zone *przs[], uint *c, uint max,
+ramoops_get_next_prz(struct persistent_ram_zone *przs[], uint *c,
u64 *id, enum pstore_type_id *typep, bool update)
{
struct persistent_ram_zone *prz;
int i = (*c)++;
/* Give up if we never existed or have hit the end. */
- if (!przs || i >= max)
+ if (!przs)
return NULL;
prz = przs[i];
@@ -254,8 +254,7 @@ static ssize_t ramoops_pstore_read(struct pstore_record
*record)
/* Find the next valid persistent_ram_zone for DMESG */
while (cxt->dump_read_cnt < cxt->max_dump_cnt && !prz) {
prz = ramoops_get_next_prz(cxt->dprzs, >dump_read_cnt,
- cxt->max_dump_cnt, >id,
- >type, 1);
+ >id, >type, 1);
if (!prz_ok(prz))
continue;
header_length = ramoops_read_kmsg_hdr(persistent_ram_old(prz),
@@ -271,17 +270,17 @@ static ssize_t ramoops_pstore_read(struct pstore_record
*record)
if (!prz_ok(prz))
prz = ramoops_get_next_prz(>cprz, >console_read_cnt,
- 1, >id, >type, 0);
+ >id, >type, 0);
if (!prz_ok(prz))
prz = ramoops_get_next_prz(>mprz, >pmsg_read_cnt,
- 1, >id, >type, 0);
+ >id, >type, 0);
/* ftrace is last since it may want to dynamically allocate memory. */
if (!prz_ok(prz)) {
if (!(cxt->flags & RAMOOPS_FLAG_FTRACE_PER_CPU)) {
prz = ramoops_get_next_prz(cxt->fprzs,
- >ftrace_read_cnt, 1, >id,
+ >ftrace_read_cnt, >id,
>type, 0);
} else {
/*
@@ -299,7 +298,6 @@ static ssize_t ramoops_pstore_read(struct pstore_record
*record)
while (cxt->ftrace_read_cnt < cxt->max_ftrace_cnt) {
prz_next = ramoops_get_next_prz(cxt->fprzs,
>ftrace_read_cnt,
- cxt->max_ftrace_cnt,
>id,
>type, 0);
--
2.19.1.568.g152ad8e336-goog
