by Ofir Bitton

[permalink] [raw]

Subject: Re: [PATCH 13/13] accel/habanalabs: modify pci health check

On 24/02/2024 1:32, Carl Vanderlip wrote:
>
> On 2/20/2024 8:01 AM, Oded Gabbay wrote:
>> From: Ofir Bitton <[email protected]>
>>
>> Today we read PCI VENDOR-ID in order to make sure PCI link is
>> healthy. Apparently the VENDOR-ID might be stored on host and
>> hence, when we read it we might not access the PCI bus.
>> In order to make sure PCI health check is reliable, we will start
>> checking the DEVICE-ID instead.
>
> What's keeping some system from caching that as well?

The PCI Controllers/switches we use in Gaudi family products might cache
only the VENDOR-ID and not the DEVICE-ID.

>
> Since this is checking for PCIe link health, it will be 0xFF when bad.
> Checking some part of Config Space that is writable would be more reliable.

Generally speaking I agree but there is no product in the Gaudi family
with a 0xFF DEVICE-ID (nor there will be), so I think this approach is
good enough for our use-case.

--
Ofir

>
> -Carl V.

2024-02-25 11:44:17

by Ohad Sharabi

[permalink] [raw]

Subject: Re: [PATCH 01/13] accel/habanalabs/gaudi2: use single function to compare FW versions

On 24/02/2024 0:38, Carl Vanderlip wrote:
> [You don't often get email from [email protected]. Learn why this
> is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> On 2/20/2024 8:01 AM, Oded Gabbay wrote:> From: Ohad Sharabi
> <[email protected]>
> >
> > Currently, the code contains 2 types of FW version comparison
> functions:
> > - hl_is_fw_sw_ver_[below/equal_or_greater]()
> > - gaudi2 specific function of the type
> >    gaudi2_is_fw_ver_[below/above]x_y_z()
> >
> > Moreover, some functions use the inner FW version which should be only
> > stage during development but not version dependencies.
> >
> > Finally, some tests are done to deprecated FW version to which LKD
> > should hold no compatibility.
> >
> > This commit aligns all APIs to a single function that just compares the
> > version and return an integers indicator (similar in some way to
> > strcmp()).
> >
> > In addition, this generic function now considers also the sub-minor FW
> > version and also remove dead code resulting in deprecated FW versions
> > compatibility.
> >
> > Signed-off-by: Ohad Sharabi <[email protected]>
> > Reviewed-by: Oded Gabbay <[email protected]>
> > Signed-off-by: Oded Gabbay <[email protected]>
> > ---
> >   drivers/accel/habanalabs/common/firmware_if.c | 25 ++++++++
> >   drivers/accel/habanalabs/common/habanalabs.h | 20 +------
> >   drivers/accel/habanalabs/gaudi2/gaudi2.c      | 57
> +++----------------
> >   3 files changed, 34 insertions(+), 68 deletions(-)
> >
> ...
> > diff --git a/drivers/accel/habanalabs/gaudi2/gaudi2.c
> b/drivers/accel/habanalabs/gaudi2/gaudi2.c
> > index 1f061209ae21..4a0917aa4dd7 100644
> > --- a/drivers/accel/habanalabs/gaudi2/gaudi2.c
> > +++ b/drivers/accel/habanalabs/gaudi2/gaudi2.c
> > @@ -2601,6 +2601,8 @@ static int gaudi2_set_fixed_properties(struct
> hl_device *hdev)
> >
> >      prop->hbw_flush_reg = mmPCIE_WRAP_SPECIAL_GLBL_SPARE_0;
> >
> > +    prop->supports_advanced_cpucp_rc = true;
> > +
> >      return 0;
> >
> >   free_qprops:
> > @@ -3308,8 +3310,6 @@ static int gaudi2_late_init(struct hl_device
> *hdev)
> >      struct gaudi2_device *gaudi2 = hdev->asic_specific;
> >      int rc;
> >
> > -    hdev->asic_prop.supports_advanced_cpucp_rc = true;
> > -
> >      rc = hl_fw_send_pci_access_msg(hdev,
> CPUCP_PACKET_ENABLE_PCI_ACCESS,
> > gaudi2->virt_msix_db_dma_addr);
> >      if (rc) {

Carl,

Sure, we'll split the patches

Ohad

>
> Is this change in support of the others in this patch? Feels like this
> should be more than one patch (adding new version_cmp, removing old
> checks).
>
> -Carl V.