Re: [PATCH V2] ieee1275/ofdisk: vscsi lun handling on lun len

[Daniel Kiper]

On Mon, Nov 27, 2023 at 06:07:42PM +0530, Mukesh Kumar Chaurasiya wrote:
> The information about "vscsi-report-luns" data is a list of disk details
> with pairs of memory addresses and lengths.

Not lengths but counts of entries.

>                   8 bytes     8 bytes
> lun-addr  --->   ------------------------              8 bytes
>         ^        |  buf-addr |  buf-len | --------> -------------
>         |        ------------------------           |   lun     |
>         |        |  buf-addr |  buf-len | ----|     -------------
>      "len"       ------------------------     |     |  ...      |
>         |        |    ...               |     |     -------------
>         |        ------------------------     |     |   lun     |
>         |        |  buf-addr |  buf-len |     |     -------------
>         V        ------------------------     |
>                                               |---> -------------
>                                                     |   lun     |
>                                                     -------------
>                                                     |  ...      |
>                                                     -------------
>                                                     |   lun     |
>                                                     -------------
> The way the expression "(args.table + 4 + 8 * i)" is used is incorrect and

I would drop quotation marks from here.

> can be confusing. The list of LUNs doesn't end with NULL, indicated by
> "while (*ptr)". Usually, this loop doesn't process any LUNs because it ends
> before checking any as first reported LUN is likely to be "0". The list of

... ditto...

> LUNs ends based on its length, not by a NULL value.
>
> Signed-off-by: Mukesh Kumar Chaurasiya <mchauras@linux.ibm.com>
> ---
>  grub-core/disk/ieee1275/ofdisk.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/grub-core/disk/ieee1275/ofdisk.c 
> b/grub-core/disk/ieee1275/ofdisk.c
> index c6cba0c8a..9cd8898f1 100644
> --- a/grub-core/disk/ieee1275/ofdisk.c
> +++ b/grub-core/disk/ieee1275/ofdisk.c
> @@ -222,8 +222,12 @@ dev_iterate (const struct grub_ieee1275_devalias *alias)
>       grub_ieee1275_cell_t table;
>        }
>        args;
> +      struct lun_buf {
> +     grub_uint64_t *buf_addr;

This will not work on 32-bit architectures.

> +     grub_uint64_t buf_len;

Again, this is not length. It is number of entries.

> +      } *tbl;

Why do you define this struct here? Do not you have its proper
definition in currently existing code? Even if not I would define it
properly outside of this function because somebody may need it later.

Daniel