Hi Mariusz,
I reply my comments in line.
> 2022年4月19日 22:37，Mariusz Tkaczyk <mariusz.tkaczyk@xxxxxxxxxxxxxxx> 写道：
>
> The routine was added to remove unnecessary get_imsm_dev() and
> get_imsm_map() calls, used only to determine disk slot.
>
> Additionally, enum for IMSM return statues was added for further usage.
>
> Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@xxxxxxxxxxxxxxx>
> ---
> super-intel.c | 46 +++++++++++++++++++++++++++++++++++-----------
> 1 file changed, 35 insertions(+), 11 deletions(-)
>
> diff --git a/super-intel.c b/super-intel.c
> index d5fad102..c16251c8 100644
> --- a/super-intel.c
> +++ b/super-intel.c
> @@ -366,6 +366,17 @@ struct migr_record {
> };
> ASSERT_SIZE(migr_record, 128)
>
> +/**
> + * enum imsm_status - internal IMSM return values representation.
> + * @STATUS_OK: function succeeded.
> + * @STATUS_ERROR: General error ocurred (not specified).
> + */
> +enum {
> + IMSM_STATUS_OK = 0,
> + IMSM_STATUS_ERROR = -1,
> +
> +} imsm_status;
To minimize code reading confusion, it might be better to declare IMSM_STATUS_ERROR before IMSM_STATUS_OK, since IMSM_STATUS_OK is negative value.
For the rested part, they look good to me.
Acked-by: Coly Li <colyli@xxxxxxx>
Thanks.
Coly Li
> +
> struct md_list {
> /* usage marker:
> * 1: load metadata
> @@ -1151,7 +1162,7 @@ static void set_imsm_ord_tbl_ent(struct imsm_map *map, int slot, __u32 ord)
> map->disk_ord_tbl[slot] = __cpu_to_le32(ord);
> }
>
> -static int get_imsm_disk_slot(struct imsm_map *map, unsigned idx)
> +static int get_imsm_disk_slot(struct imsm_map *map, const unsigned int idx)
> {
> int slot;
> __u32 ord;
> @@ -1162,7 +1173,7 @@ static int get_imsm_disk_slot(struct imsm_map *map, unsigned idx)
> return slot;
> }
>
> - return -1;
> + return IMSM_STATUS_ERROR;
> }
>
> static int get_imsm_raid_level(struct imsm_map *map)
> @@ -1177,6 +1188,23 @@ static int get_imsm_raid_level(struct imsm_map *map)
> return map->raid_level;
> }
>
> +/**
> + * get_disk_slot_in_dev() - retrieve disk slot from &imsm_dev.
> + * @super: &intel_super pointer, not NULL.
> + * @dev_idx: imsm device index.
> + * @idx: disk index.
> + *
> + * Return: Slot on success, IMSM_STATUS_ERROR otherwise.
> + */
> +static int get_disk_slot_in_dev(struct intel_super *super, const __u8 dev_idx,
> + const unsigned int idx)
> +{
> + struct imsm_dev *dev = get_imsm_dev(super, dev_idx);
> + struct imsm_map *map = get_imsm_map(dev, MAP_0);
> +
> + return get_imsm_disk_slot(map, idx);
> +}
> +
> static int cmp_extent(const void *av, const void *bv)
> {
> const struct extent *a = av;
> @@ -1193,13 +1221,9 @@ static int count_memberships(struct dl *dl, struct intel_super *super)
> int memberships = 0;
> int i;
>
> - for (i = 0; i < super->anchor->num_raid_devs; i++) {
> - struct imsm_dev *dev = get_imsm_dev(super, i);
> - struct imsm_map *map = get_imsm_map(dev, MAP_0);
> -
> - if (get_imsm_disk_slot(map, dl->index) >= 0)
> + for (i = 0; i < super->anchor->num_raid_devs; i++)
> + if (get_disk_slot_in_dev(super, i, dl->index) >= 0)
> memberships++;
> - }
>
> return memberships;
> }
> @@ -1909,6 +1933,7 @@ void examine_migr_rec_imsm(struct intel_super *super)
>
> /* first map under migration */
> map = get_imsm_map(dev, MAP_0);
> +
> if (map)
> slot = get_imsm_disk_slot(map, super->disks->index);
> if (map == NULL || slot > 1 || slot < 0) {
> @@ -9629,10 +9654,9 @@ static int apply_update_activate_spare(struct imsm_update_activate_spare *u,
> /* count arrays using the victim in the metadata */
> found = 0;
> for (a = active_array; a ; a = a->next) {
> - dev = get_imsm_dev(super, a->info.container_member);
> - map = get_imsm_map(dev, MAP_0);
> + int dev_idx = a->info.container_member;
>
> - if (get_imsm_disk_slot(map, victim) >= 0)
> + if (get_disk_slot_in_dev(super, dev_idx, victim) >= 0)
> found++;
> }
>
> --
> 2.26.2
>
