linux-kernel - Re: [PATCH v10 8/8] PCI: Change the type of probe argument in reset functions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 28 Jul 2021 23:05:14 +0530
From:   Amey Narkhede <ameynarkhede03@...il.com>
To:     Bjorn Helgaas <helgaas@...nel.org>
Cc:     alex.williamson@...hat.com,
        Raphael Norwitz <raphael.norwitz@...anix.com>,
        linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
        kw@...ux.com, Shanker Donthineni <sdonthineni@...dia.com>,
        Sinan Kaya <okaya@...nel.org>, Len Brown <lenb@...nel.org>,
        "Rafael J . Wysocki" <rjw@...ysocki.net>
Subject: Re: [PATCH v10 8/8] PCI: Change the type of probe argument in reset
 functions

On 21/07/27 05:22PM, Bjorn Helgaas wrote:
> On Fri, Jul 09, 2021 at 06:08:13PM +0530, Amey Narkhede wrote:
> > Introduce a new enum pci_reset_mode_t to make the context of probe argument
> > in reset functions clear and the code easier to read.  Change the type of
> > probe argument in functions which implement reset methods from int to
> > pci_reset_mode_t to make the intent clear.
>
> Not sure adding an enum and a PCI_RESET_MODE_MAX seems worth it to me.
> It's really a boolean parameter, and I'd be happy to change it to a
> bool.  But I don't think it's worth checking against
> PCI_RESET_MODE_MAX unless we need more than two options.
>
Is it okay to use PCI_RESET_PROBE and PCI_RESET_DO_RESET as bool.
That would be less confusing than directly using true/false.

Thanks,
Amey

> > Add a new line in return statement of pci_reset_bus_function().
>
> Drop this line, since I don't think it's correct and there's no need
> to describe whitespace changes anyway.
>
> > Suggested-by: Alex Williamson <alex.williamson@...hat.com>
> > Suggested-by: Krzysztof Wilczyński <kw@...ux.com>
> > Signed-off-by: Amey Narkhede <ameynarkhede03@...il.com>
>
> > @@ -306,7 +306,7 @@ static int nitrox_device_flr(struct pci_dev *pdev)
> >  		return -ENOMEM;
> >  	}
> >
> > -	pcie_reset_flr(pdev, 0);
> > +	pcie_reset_flr(pdev, PCI_RESET_DO_RESET);
> >
> >  	pci_restore_state(pdev);
> >
>
> > @@ -526,7 +526,7 @@ static void octeon_destroy_resources(struct octeon_device *oct)
> >  			oct->irq_name_storage = NULL;
> >  		}
> >  		/* Soft reset the octeon device before exiting */
> > -		if (!pcie_reset_flr(oct->pci_dev, 1))
> > +		if (!pcie_reset_flr(oct->pci_dev, PCI_RESET_PROBE))
> >  			octeon_pci_flr(oct);
> >  		else
> >  			cn23xx_vf_ask_pf_to_do_flr(oct);
>
> > +typedef enum pci_reset_mode {
> > +	PCI_RESET_DO_RESET,
> > +	PCI_RESET_PROBE,
> > +	PCI_RESET_MODE_MAX,
> > +} pci_reset_mode_t;