Hello Martin,
Thanks for the review!
On Fri, 8 Dec 2023 18:30:01 +0100
Martin Mareš <mj@xxxxxx> wrote:
> > -all: lib/$(PCIIMPLIB) lspci$(EXEEXT) setpci$(EXEEXT) example$(EXEEXT) lspci.8 setpci.8 pcilib.7 pci.ids.5 update-pciids update-pciids.8 $(PCI_IDS)
> > +all: lib/$(PCIIMPLIB) lspci$(EXEEXT) setpci$(EXEEXT) example$(EXEEXT) lspci.8 setpci.8 pcilib.7 pci.ids.5 update-pciids update-pciids.8 $(PCI_IDS) lmr_lib/liblmr.a
>
> Is there any advantage with building LMR as a library instead of linking all
> the object files with the margining utility?
Actually, there are no advantages, I just thought that the Makefiles would
look more neat with this approach. I will redo the linking to make it
consistent with the lspci building.
> > +bool margin_prep_dev(struct margin_dev *dev)
> > +{
> > + struct pci_cap *pcie = pci_find_cap(dev->dev, PCI_CAP_ID_EXP, PCI_CAP_NORMAL);
>
> What if it doesn't exist?
Nothing good at all. I will add more checks.
> > --- /dev/null
> > +++ b/lmr_lib/margin_hw.c
> > @@ -0,0 +1,85 @@
> > +#include <stdio.h>
> > +#include <string.h>
> > +#include <stdlib.h>
>
> Generally: Please add a comment to every source file, which explains the
> purpose of the file and contains a copyright notice. See existing files
> for the recommeneded format.
>
> > + uint8_t down_type = pci_read_byte(down_port, PCI_HEADER_TYPE) & 0x7F;
> > + uint8_t down_sec = pci_read_byte(down_port, PCI_SECONDARY_BUS);
> > + uint8_t down_dir = (pci_read_word(down_port, cap->addr + PCI_EXP_FLAGS) & PCI_EXP_FLAGS_TYPE) >> 4;
>
> I would prefer using libpci types (u8, u32 etc.).
>
> > + if (!(down_sec == up_port->bus && down_type == 1
>
> Please avoid whitespace at the end of line.
>
> > diff --git a/lmr_lib/margin_hw.h b/lmr_lib/margin_hw.h
> > new file mode 100644
> > index 0000000..a436d4b
> > --- /dev/null
> > +++ b/lmr_lib/margin_hw.h
> > @@ -0,0 +1,39 @@
> > +#ifndef _MARGIN_HW_H
> > +#define _MARGIN_HW_H
> > +
> > +#include <stdbool.h>
> > +#include <stdint.h>
> > +
> > +#include "../lib/pci.h"
>
> Please do not use relative paths to libpci header files.
> Instead, supply proper include path to CC in the Makefile.
>
> > +/*PCI Device wrapper for margining functions*/
>
> Please surround "/*" and "*/" by spaces as in existing source files.
Got it, I'll rework the code.
Best regards,
Nikita Proshkin
