| Message ID | 20230719080526.2436951-1-ruansy.fnst@fujitsu.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Ira Weiny |
| Headers | show |
| Series | nfit: remove redundant list_for_each_entry | expand |
On Wed, Jul 19, 2023 at 04:05:26PM +0800, Shiyang Ruan wrote: > The first for_each only do acpi_nfit_init_ars() for NFIT_SPA_VOLATILE > and NFIT_SPA_PM, which can be moved to next one. Can the result of nfit_spa_type(nfit_spa->spa) change as a result of the first switch statement? That would be a reason why they are separate. Alison > > Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com> > --- > drivers/acpi/nfit/core.c | 8 -------- > 1 file changed, 8 deletions(-) > > diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c > index 07204d482968..4090a0a0505c 100644 > --- a/drivers/acpi/nfit/core.c > +++ b/drivers/acpi/nfit/core.c > @@ -2971,14 +2971,6 @@ static int acpi_nfit_register_regions(struct acpi_nfit_desc *acpi_desc) > case NFIT_SPA_VOLATILE: > case NFIT_SPA_PM: > acpi_nfit_init_ars(acpi_desc, nfit_spa); > - break; > - } > - } > - > - list_for_each_entry(nfit_spa, &acpi_desc->spas, list) { > - switch (nfit_spa_type(nfit_spa->spa)) { > - case NFIT_SPA_VOLATILE: > - case NFIT_SPA_PM: > /* register regions and kick off initial ARS run */ > rc = ars_register(acpi_desc, nfit_spa); > if (rc) > -- > 2.41.0 >
在 2023/7/25 6:47, Alison Schofield 写道: > On Wed, Jul 19, 2023 at 04:05:26PM +0800, Shiyang Ruan wrote: >> The first for_each only do acpi_nfit_init_ars() for NFIT_SPA_VOLATILE >> and NFIT_SPA_PM, which can be moved to next one. > > Can the result of nfit_spa_type(nfit_spa->spa) change as a result of > the first switch statement? That would be a reason why they are separate. nfit_spa_type() just gets the type of *spa by querying a type-uuid table. Also, according to the code shown below, we can find that it doesn't change anything. int nfit_spa_type(struct acpi_nfit_system_address *spa) { guid_t guid; int i; import_guid(&guid, spa->range_guid); for (i = 0; i < NFIT_UUID_MAX; i++) if (guid_equal(to_nfit_uuid(i), &guid)) return i; return -1; } -- Thanks, Ruan. > > Alison > >> >> Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com> >> --- >> drivers/acpi/nfit/core.c | 8 -------- >> 1 file changed, 8 deletions(-) >> >> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c >> index 07204d482968..4090a0a0505c 100644 >> --- a/drivers/acpi/nfit/core.c >> +++ b/drivers/acpi/nfit/core.c >> @@ -2971,14 +2971,6 @@ static int acpi_nfit_register_regions(struct acpi_nfit_desc *acpi_desc) >> case NFIT_SPA_VOLATILE: >> case NFIT_SPA_PM: >> acpi_nfit_init_ars(acpi_desc, nfit_spa); >> - break; >> - } >> - } >> - >> - list_for_each_entry(nfit_spa, &acpi_desc->spas, list) { >> - switch (nfit_spa_type(nfit_spa->spa)) { >> - case NFIT_SPA_VOLATILE: >> - case NFIT_SPA_PM: >> /* register regions and kick off initial ARS run */ >> rc = ars_register(acpi_desc, nfit_spa); >> if (rc) >> -- >> 2.41.0 >>
On Tue, Jul 25, 2023 at 01:33:18PM +0800, Shiyang Ruan wrote: > > > 在 2023/7/25 6:47, Alison Schofield 写道: > > On Wed, Jul 19, 2023 at 04:05:26PM +0800, Shiyang Ruan wrote: > > > The first for_each only do acpi_nfit_init_ars() for NFIT_SPA_VOLATILE > > > and NFIT_SPA_PM, which can be moved to next one. > > > > Can the result of nfit_spa_type(nfit_spa->spa) change as a result of > > the first switch statement? That would be a reason why they are separate. > > nfit_spa_type() just gets the type of *spa by querying a type-uuid table. > Also, according to the code shown below, we can find that it doesn't change > anything. > > int nfit_spa_type(struct acpi_nfit_system_address *spa) > { > guid_t guid; > int i; > > import_guid(&guid, spa->range_guid); > for (i = 0; i < NFIT_UUID_MAX; i++) > if (guid_equal(to_nfit_uuid(i), &guid)) > return i; > return -1; > } > Hi Ruan, I see that. I was questioning if the type change as a *result* of the first switch statement, which does that acpi_nfi_init_ars(). I don't think it does. I'm only asking if you proved the correctness of the change because I'm guessing this change is tested by inspection only. Maybe not. Thanks, Alison > -- > Thanks, > Ruan. > > > > > Alison > > > > > > > > Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com> > > > --- > > > drivers/acpi/nfit/core.c | 8 -------- > > > 1 file changed, 8 deletions(-) > > > > > > diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c > > > index 07204d482968..4090a0a0505c 100644 > > > --- a/drivers/acpi/nfit/core.c > > > +++ b/drivers/acpi/nfit/core.c > > > @@ -2971,14 +2971,6 @@ static int acpi_nfit_register_regions(struct acpi_nfit_desc *acpi_desc) > > > case NFIT_SPA_VOLATILE: > > > case NFIT_SPA_PM: > > > acpi_nfit_init_ars(acpi_desc, nfit_spa); > > > - break; > > > - } > > > - } > > > - > > > - list_for_each_entry(nfit_spa, &acpi_desc->spas, list) { > > > - switch (nfit_spa_type(nfit_spa->spa)) { > > > - case NFIT_SPA_VOLATILE: > > > - case NFIT_SPA_PM: > > > /* register regions and kick off initial ARS run */ > > > rc = ars_register(acpi_desc, nfit_spa); > > > if (rc) > > > -- > > > 2.41.0 > > >
diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c index 07204d482968..4090a0a0505c 100644 --- a/drivers/acpi/nfit/core.c +++ b/drivers/acpi/nfit/core.c @@ -2971,14 +2971,6 @@ static int acpi_nfit_register_regions(struct acpi_nfit_desc *acpi_desc) case NFIT_SPA_VOLATILE: case NFIT_SPA_PM: acpi_nfit_init_ars(acpi_desc, nfit_spa); - break; - } - } - - list_for_each_entry(nfit_spa, &acpi_desc->spas, list) { - switch (nfit_spa_type(nfit_spa->spa)) { - case NFIT_SPA_VOLATILE: - case NFIT_SPA_PM: /* register regions and kick off initial ARS run */ rc = ars_register(acpi_desc, nfit_spa); if (rc)
The first for_each only do acpi_nfit_init_ars() for NFIT_SPA_VOLATILE and NFIT_SPA_PM, which can be moved to next one. Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com> --- drivers/acpi/nfit/core.c | 8 -------- 1 file changed, 8 deletions(-)