Re: [PATCH 1/1] mdadm: enable Intel Alderlake RST VMD configuration

[Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>]

On Thu, 11 Aug 2022 21:38:59 +0200
Oldřich Jedlička <oldium.pro@gmail.com> wrote:

> Hi Mariusz,
> 
> čt 11. 8. 2022 v 12:18 odesílatel Mariusz Tkaczyk
> <mariusz.tkaczyk@linux.intel.com> napsal:
> >
> > Hello Oldřich,
> >
> > On Fri, 5 Aug 2022 14:50:36 +0200
> > Oldřich Jedlička <oldium.pro@gmail.com> wrote:
> >  
> > > pá 5. 8. 2022 v 13:56 odesílatel Kinga Tanska
> > > <kinga.tanska@linux.intel.com> napsal:  
> > > >
> > > > On Fri,  5 Aug 2022 12:05:45 +0200
> > > > Oldřich Jedlička <oldium.pro@gmail.com> wrote:
> > > >  
> > > > > Alderlake changed UEFI variable name to 'RstVmdV' also and for VMD
> > > > > devices, so check the updated name for VMD devices like it is done in
> > > > > the SATA case.  
> >
> > Alderlake is RST so it doesn't changed, it is different. It is a support for
> > RST family product.  
> > > > >
> > > > > Signed-off-by: Oldřich Jedlička <oldium.pro@gmail.com>
> > > > > ---
> > > > >  platform-intel.c | 19 ++++++++++++-------
> > > > >  1 file changed, 12 insertions(+), 7 deletions(-)
> > > > >
> > > > > diff --git a/platform-intel.c b/platform-intel.c
> > > > > index a4d55a3..2f8e6af 100644
> > > > > --- a/platform-intel.c
> > > > > +++ b/platform-intel.c
> > > > > @@ -512,8 +512,8 @@ static const struct imsm_orom
> > > > > *find_imsm_hba_orom(struct sys_dev *hba) #define AHCI_PROP "RstSataV"
> > > > >  #define AHCI_SSATA_PROP "RstsSatV"
> > > > >  #define AHCI_TSATA_PROP "RsttSatV"
> > > > > -#define AHCI_RST_PROP "RstVmdV"
> > > > > -#define VMD_PROP "RstUefiV"
> > > > > +#define RST_VMD_PROP "RstVmdV"
> > > > > +#define RST_UEFI_PROP "RstUefiV"  
> >
> > There are two products RSTe/ VROC and RST.  Here you are adding support for
> > RST platform. Please name it accordingly (yeah, I know that naming is
> > confusing). I propose:
> >
> > #define RST_VMD_PROP "RstVmdV"
> > #define VROC_VMD_PROP "RstUefiV"
> >  
> > > > >
> > > > >  #define VENDOR_GUID \
> > > > >       EFI_GUID(0x193dfefa, 0xa445, 0x4302, 0x99, 0xd8, 0xef, 0x3a,
> > > > > 0xad, 0x1a, 0x04, 0xc6) @@ -607,7 +607,8 @@ const struct imsm_orom
> > > > > *find_imsm_efi(struct sys_dev *hba) struct orom_entry *ret;
> > > > >       static const char * const sata_efivars[] = {AHCI_PROP,
> > > > > AHCI_SSATA_PROP, AHCI_TSATA_PROP,
> > > > > -                                                 AHCI_RST_PROP};
> > > > > +                                                 RST_VMD_PROP};
> > > > > +     static const char * const vmd_efivars[] = {RST_UEFI_PROP,
> > > > > RST_VMD_PROP}; unsigned long i;
> > > > >
> > > > >       if (check_env("IMSM_TEST_AHCI_EFI") ||
> > > > > check_env("IMSM_TEST_SCU_EFI")) @@ -640,10 +641,14 @@ const struct
> > > > > imsm_orom *find_imsm_efi(struct sys_dev *hba)
> > > > >               break;
> > > > >       case SYS_DEV_VMD:
> > > > > -             if (!read_efi_variable(&orom, sizeof(orom), VMD_PROP,
> > > > > -                                    VENDOR_GUID))
> > > > > -                     break;
> > > > > -             return NULL;
> > > > > +             for (i = 0; i < ARRAY_SIZE(vmd_efivars); i++) {
> > > > > +                     if (!read_efi_variable(&orom, sizeof(orom),
> > > > > +                                             vmd_efivars[i],
> > > > > VENDOR_GUID))
> > > > > +                             break;
> > > > > +             }
> > > > > +             if (i == ARRAY_SIZE(vmd_efivars))
> > > > > +                     return NULL;
> > > > > +             break;
> > > > >       default:
> > > > >               return NULL;
> > > > >       }  
> > > >
> > > > Hi,
> > > >
> > > > please have a look at the following mail:
> > > > https://marc.info/?l=linux-raid&m=165969352101643&w=2  
> > >
> > >
> > > Hi, the described issue applies specifically in the SYS_DEV_SATA (SATA
> > > configuration) case, so it should not apply to SYS_DEV_VMD (VMD
> > > configuration) one.
> > >
> > > For me, the platform output looks reasonable (I have RAID 0 active):
> > >  
> > > #> sudo mdadm --detail-platform  
> > >        Platform : Intel(R) Rapid Storage Technology
> > >         Version : 19.0.7.5579
> > >     RAID Levels : raid0 raid1 raid10 raid5
> > >     Chunk Sizes : 4k 8k 16k 32k 64k 128k
> > >     2TB volumes : supported
> > >       2TB disks : supported
> > >       Max Disks : 32
> > >     Max Volumes : 2 per array, 4 per controller
> > >  3rd party NVMe : supported
> > >  I/O Controller : /sys/devices/pci0000:00/0000:00:0e.0 (VMD)
> > >  NVMe under VMD : /dev/nvme0n1 (S6P1NS0T318266R)
> > >  NVMe under VMD : /dev/nvme1n1 (S6P1NS0T318223V)
> > >
> > > Without the patch the platform isn't even recognized. Common to both
> > > changes is the usage of the new UEFI variable 'RstVmdV', not the changes
> > > to the controller.
> > >  
> >
> > This version is different than one sent by Coly. We will test that to see
> > if it doesn't cause regression in our VROC product.
> > I noted  some nits, we will test this version anyway.  
> 
> Should I update the description (to just mention adding support for
> Alderlake RST on VMD platform), change the defines in the patch, and
> send V2, or you will send (possibly) updated version after your
> testing?
> 
> The patch was tested on my Dell Precision 3570 notebook with Intel
> i7-1255U CPU and two 2TB NVMe disks.
> 

Please do. This is your patch, so I don't want to take a your glory :)

Thanks,
Mariusz