| 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
| ||
|
Date: Sun, 14 Jan 2018 11:05:15 +0530
From: poza@...eaurora.org
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: Bjorn Helgaas <bhelgaas@...gle.com>,
Philippe Ombredanne <pombredanne@...b.com>,
Thomas Gleixner <tglx@...utronix.de>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Kate Stewart <kstewart@...uxfoundation.org>,
linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
Dongdong Liu <liudongdong3@...wei.com>,
Gabriele Paoloni <gabriele.paoloni@...wei.com>,
Keith Busch <keith.busch@...el.com>, Wei Zhang <wzhang@...com>,
Sinan Kaya <okaya@...eaurora.org>,
Timur Tabi <timur@...eaurora.org>
Subject: Re: [PATCH v3 1/3] PCI/AER: factor out error reporting from AER
On 2018-01-13 06:27, Bjorn Helgaas wrote:
> On Mon, Jan 08, 2018 at 01:25:03PM +0530, Oza Pawandeep wrote:
>> This patch factors out error reporting callbacks, which are currently
>> tightly coupled with AER.
>> DPC should be able to call these callbacks when DPC trigger event
>> occurs.
>>
>> Signed-off-by: Oza Pawandeep <poza@...eaurora.org>
>>
>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>> index 6402f7f..fd053e5 100644
>> --- a/drivers/acpi/apei/ghes.c
>> +++ b/drivers/acpi/apei/ghes.c
>> @@ -462,7 +462,7 @@ static void ghes_do_proc(struct ghes *ghes,
>> * use, so treat it as a fatal AER error.
>> */
>> if (gdata->flags & CPER_SEC_RESET)
>> - aer_severity = AER_FATAL;
>> + aer_severity = PCI_ERR_AER_FATAL;
>
> Please split the s/AER_FATAL/PCI_ERR_AER_FATAL/ changes into a
> separate patch to reduce the size of this patch.
>
will do.
> I would name them PCI_ERR_FATAL and PCI_ERR_NONFATAL because that
> matches the usage in the spec, e.g., PCIe r4.0, sec 6.2.2, and the
> symbols like PCI_ERR_UNC_UND in pci_regs.h.
>
ok will work on that.
>> diff --git a/drivers/pci/pcie/pcie-err.c b/drivers/pci/pcie/pcie-err.c
>> new file mode 100644
>> index 0000000..a76a8bf
>> --- /dev/null
>> +++ b/drivers/pci/pcie/pcie-err.c
>> @@ -0,0 +1,335 @@
>> +/*
>> + * Copyright (c) 2017, The Linux Foundation. All rights reserved.
>
> Somebody already mentioned using the SPDX thing, which I think you
> should do.
>
> But I'm confused about this copyright line. As far as I can tell,
> this basically moves code from aerdrv_core.c to pcie-err.c, which is
> not enough to change the copyright ownership. But it drops the
> copyright lines from aerdrv.core.c and replaces them with "(c) 2017,
> The Linux Foundation". ?? Where did that come from?
>
> If you *add* something non-trivial, I think it's OK to add your own
> new copyright info (though I don't think this is really necessary),
> but I don't think we should *remove* information about other copyright
> owners.
>
sure will keep original copyright owners info, and SPDX as well.
>> + * This program is free software; you can redistribute it and/or
>> modify
>> + * it under the terms of the GNU General Public License version 2 and
>> + * only version 2 as published by the Free Software Foundation.
>> +
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/pci.h>
>> +#include <linux/module.h>
>> +#include <linux/pci.h>
>> +#include <linux/kernel.h>
>> +#include <linux/errno.h>
>> +#include <linux/aer.h>
>> +#include <linux/pcieport_if.h>
>> +#include "portdrv.h"
>> +
>> +static DEFINE_MUTEX(pci_err_recovery_lock);
>> +
>> +pci_ers_result_t pci_merge_result(enum pci_ers_result orig,
>> + enum pci_ers_result new)
>
> Please do all the renames, e.g., s/merge_result/pci_merge_result/, in
> a separate patch, followed by one that only moves code between files.
>
> These initial ones will seem trivial, and they are, which is perfect.
> That makes them easy to review, and it will make the "interesting"
> patches much smaller and also easier to review.
>
ok all the renamed will be made in a separate patch.
> The you can follow up with more patches that do things like add the
> mutex. It's too hard to review (or even notice) things like that when
> everything is squashed together.
>
sure.
>> diff --git a/include/linux/aer.h b/include/linux/aer.h
>> index 8f87bbe..3eac8ed 100644
>> --- a/include/linux/aer.h
>> +++ b/include/linux/aer.h
>> @@ -11,10 +11,6 @@
>> #include <linux/errno.h>
>> #include <linux/types.h>
>>
>> -#define AER_NONFATAL 0
>> -#define AER_FATAL 1
>> -#define AER_CORRECTABLE 2
>> -
>> struct pci_dev;
>>
>> struct aer_header_log_regs {
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index c170c92..083408e 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -739,6 +739,10 @@ struct pci_error_handlers {
>> void (*resume)(struct pci_dev *dev);
>> };
>>
>> +struct pci_err_broadcast_data {
>> + enum pci_channel_state state;
>> + enum pci_ers_result result;
>> +};
>
> Only used in pcie-err.c; should be declared there.
>
>> struct module;
>> struct pci_driver {
>> @@ -1998,6 +2002,23 @@ static inline resource_size_t
>> pci_iov_resource_size(struct pci_dev *dev, int res
>> void pci_hp_remove_module_link(struct pci_slot *pci_slot);
>> #endif
>>
>> +#define PCI_ERR_AER_NONFATAL 0
>> +#define PCI_ERR_AER_FATAL 1
>> +#define PCI_ERR_AER_CORRECTABLE 2
>
> Why do these need to be moved to include/linux/pci.h? I don't really
> want them in include/linux at all. The only uses outside drivers/pci
> are in ras_event.h and acpi/apei/ghes.c. I'd rather keep them in
> aer.h with the hope of being able to move them into drivers/pci/pci.h
> eventually.
but if I do PCI_ERR_FATAL in aer.h
how dpc can use that ?
let me see what best I can do because PCI_ERR_AER_FATAL if renamed to
PCI_ERR_FATAL
then both aer and dpc will use that.
>
>> +pci_ers_result_t pci_broadcast_error_message(struct pci_dev *dev,
>> + enum pci_channel_state state,
>> + char *error_mesg,
>> + int (*cb)(struct pci_dev *, void *));
>> +int pci_report_mmio_enabled(struct pci_dev *dev, void *data);
>> +int pci_report_slot_reset(struct pci_dev *dev, void *data);
>> +int pci_report_resume(struct pci_dev *dev, void *data);
>> +int pci_report_error_detected(struct pci_dev *dev, void *data);
>> +pci_ers_result_t pci_reset_link(struct pci_dev *dev);
>> +pci_ers_result_t pci_merge_result(enum pci_ers_result orig,
>> + enum pci_ers_result new);
>
> The above are only used in pcie-err.c and should be static and not
> declared here.
>
>> +void pci_do_recovery(struct pci_dev *dev, int severity);
>> +
>> /**
>> * pci_pcie_cap - get the saved PCIe capability offset
>> * @dev: PCI device
>> diff --git a/include/ras/ras_event.h b/include/ras/ras_event.h
>> index 9c68986..6176e90 100644
>> --- a/include/ras/ras_event.h
>> +++ b/include/ras/ras_event.h
>> @@ -316,10 +316,10 @@
>>
>> TP_printk("%s PCIe Bus Error: severity=%s, %s\n",
>> __get_str(dev_name),
>> - __entry->severity == AER_CORRECTABLE ? "Corrected" :
>> - __entry->severity == AER_FATAL ?
>> + __entry->severity == PCI_ERR_AER_CORRECTABLE ? "Corrected" :
>> + __entry->severity == PCI_ERR_AER_FATAL ?
>> "Fatal" : "Uncorrected, non-fatal",
>> - __entry->severity == AER_CORRECTABLE ?
>> + __entry->severity == PCI_ERR_AER_CORRECTABLE ?
>> __print_flags(__entry->status, "|", aer_correctable_errors) :
>> __print_flags(__entry->status, "|", aer_uncorrectable_errors))
>> );
>> --
>> Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
>> Technologies, Inc.,
>> a Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a
>> Linux Foundation Collaborative Project.
>>
Powered by blists - more mailing lists