Re: [PATCH 1/2, pci] pci: Addition of PCI_DEV_FLAGS_QUIET_PCI_REALIGN attribute to the PCI subsystem
On 03/14/2018 03:55 PM, Bjorn Helgaas wrote: As far as I'm concerned, we can just remove these messages completely. I don't think there's any real value there. After I found out that this was happening to all PCI devices on powerpc due to the __weak pcibios_default_alignment() interface (necessary for VFIO passthrough and performance), I confess that this was my first approach to this matter; however I couldn't vouch the need of these messages on other architectures. If there are no further concerns, I definitely prefer sending a second version of this patch only eliminating these messages and attesting the reason why. Thank you very much for your review Bjorn, No problem, welcome to PCI, and I hope we see more of your work! Bjorn, Awesome! A second version of this fix has been sent with a new title: "[PATCH, pci, v2] pci: Delete PCI disabling informational messages" Thanks for the review and warm welcome! -- Desnes A. Nunes do Rosário --
Re: [PATCH 1/2, pci] pci: Addition of PCI_DEV_FLAGS_QUIET_PCI_REALIGN attribute to the PCI subsystem
Hello Bjorn,
On 03/14/2018 03:06 PM, Bjorn Helgaas wrote:
On Wed, Mar 14, 2018 at 01:34:54PM -0300, Desnes A. Nunes do Rosario wrote:
Add PCI_DEV_FLAGS_QUIET_PCI_REALIGN to pci_dev_flags and use it to
silent PCI realignment messages if the flag is turned on by a driver.
Signed-off-by: Desnes A. Nunes do Rosario
---
drivers/pci/pci.c | 3 ++-
drivers/pci/setup-res.c | 3 ++-
include/linux/pci.h | 2 ++
3 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 8c71d1a66cdd..be197c944e5f 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5505,7 +5505,8 @@ void pci_reassigndev_resource_alignment(struct pci_dev
*dev)
return;
}
- pci_info(dev, "Disabling memory decoding and releasing memory resources\n");
+ if (!(dev->dev_flags & PCI_DEV_FLAGS_QUIET_PCI_REALIGN))
+ pci_info(dev, "Disabling memory decoding and releasing memory
resources\n");
pci_read_config_word(dev, PCI_COMMAND, &command);
command &= ~PCI_COMMAND_MEMORY;
pci_write_config_word(dev, PCI_COMMAND, command);
diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
index 369d48d6c6f1..00a538def763 100644
--- a/drivers/pci/setup-res.c
+++ b/drivers/pci/setup-res.c
@@ -172,7 +172,8 @@ EXPORT_SYMBOL(pci_claim_resource);
void pci_disable_bridge_window(struct pci_dev *dev)
{
- pci_info(dev, "disabling bridge mem windows\n");
+ if (!(dev->dev_flags & PCI_DEV_FLAGS_QUIET_PCI_REALIGN))
+ pci_info(dev, "disabling bridge mem windows\n");
As far as I'm concerned, we can just remove these messages completely.
I don't think there's any real value there.
After I found out that this was happening to all PCI devices on powerpc
due to the __weak
pcibios_default_alignment() interface (necessary for VFIO passthrough
and performance), I confess that this was my first approach to this
matter; however I couldn't vouch the need of these messages on other
architectures.
If there are no further concerns, I definitely prefer sending a second
version of this patch only eliminating these messages and attesting the
reason why.
Thank you very much for your review Bjorn,
--
Desnes A. Nunes do Rosário
Re: [PATCH 1/2, pci] pci: Addition of PCI_DEV_FLAGS_QUIET_PCI_REALIGN attribute to the PCI subsystem
Hello Andy,
On 03/14/2018 02:41 PM, Andy Shevchenko wrote:
On Wed, Mar 14, 2018 at 6:34 PM, Desnes A. Nunes do Rosario
wrote:
Add PCI_DEV_FLAGS_QUIET_PCI_REALIGN to pci_dev_flags and use it to
silent PCI realignment messages if the flag is turned on by a driver.
It doesn't explain "Why?"
Why the driver needs that?
I have written down the reason on the cover letter, but I concur on
creating a second version of the patch's comment. Basically, all PCI
resources on powerpc are printing out expected realignment messages
which are flooding the systems logs.
Perhaps this would be better?
---
"Some architectures such as powerpc has aligned all of its PCI resources
to its PAGE_SIZE during boot, thus the system logs will be flooded by
expected realignment messages, which can be interpreted as a false
positive for total PCI failure on the system.
[root@system user]# dmesg | grep -i disabling
[0.692270] pci :00:00.0: Disabling memory decoding and releasing
memory resources
[0.692324] pci :00:00.0: disabling bridge mem windows
[0.729134] pci 0001:00:00.0: Disabling memory decoding and releasing
memory resources
[0.737352] pci 0001:00:00.0: disabling bridge mem windows
[0.776295] pci 0002:00:00.0: Disabling memory decoding and releasing
memory resources
[0.784509] pci 0002:00:00.0: disabling bridge mem windows
... and goes on for all PCI devices ...
Thus, this patch adds PCI_DEV_FLAGS_NO_REALIGN_MSG to pci_dev_flags and
uses it to silent PCI realignment messages if the flag is turned on by a
driver.
"
---
Another approach is to increase level of the message. Would it be
accepted by you (in case Bjorn agrees)?
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -205,6 +205,8 @@ enum pci_dev_flags {
PCI_DEV_FLAGS_NO_FLR_RESET = (__force pci_dev_flags_t) (1 << 10),
/* Don't use Relaxed Ordering for TLPs directed at this device */
PCI_DEV_FLAGS_NO_RELAXED_ORDERING = (__force pci_dev_flags_t) (1 <<
11),
+ /* Silent PCI resource realignment messages */
+ PCI_DEV_FLAGS_QUIET_PCI_REALIGN = (__force pci_dev_flags_t) (1 << 12),
I would rather name it _NO_PCI_REALIGN_MSG
I concur on changing it to PCI_DEV_FLAGS_NO_REALIGN_MSG in a second
version of the patchset.
};
Thank you very much for your review,
--
Desnes A. Nunes do Rosário
Re: [PATCH, net] ibmvnic: fix firmware version when no firmware level has been provided by the VIOS server
Hello Tyrel,
I concur with your observations, but since this patch has already been
merged, I'll address them in another patch.
Thank you for your review,
On 02/01/2018 07:02 PM, Tyrel Datwyler wrote:
On 02/01/2018 10:04 AM, Desnes Augusto Nunes do Rosario wrote:
Older versions of VIOS servers do not send the firmware level in the VPD
buffer for the ibmvnic driver. Thus, not only the current message is mis-
leading but the firmware version in the ethtool will be NULL. Therefore,
this patch fixes the firmware string and its warning.
Fixes: 4e6759be28e4 ("ibmvnic: Feature implementation of VPD for the ibmvnic
driver")
Signed-off-by: Desnes A. Nunes do Rosario
---
drivers/net/ethernet/ibm/ibmvnic.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/ibm/ibmvnic.c
b/drivers/net/ethernet/ibm/ibmvnic.c
index b65f5f3ac034..2b3e71b63a7a 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -3290,7 +3290,11 @@ static void handle_vpd_rsp(union ibmvnic_crq *crq,
*/
substr = strnstr(adapter->vpd->buff, "RM", adapter->vpd->len);
if (!substr) {
- dev_info(dev, "No FW level provided by VPD\n");
+ dev_info(dev, "Warning - No FW level has been provided in the VPD
buffer by the VIOS Server\n");
+ ptr = strncpy((char *)adapter->fw_version, "N/A",
Is "N/A" the right thing to report? Would something like "Unknown" or
"Unreported" be better?
+ 3 * sizeof(char));
+ if (!ptr)
+ dev_err(dev, "Failed to inform that firmware version is
unavailable to the adapter\n");
The sentence structure here seems awkward. I would probably just get rid of
this error and this one later in the function.
dev_err(dev, "Failed to isolate FW level string\n");
Instead just check and report if adapter->fw_version == NULL in the complete:
label section.
-Tyrel
goto complete;
}
--
Desnes Augusto Nunes do Rosário
--
Linux Developer - IBM / Brazil
M.Sc. in Electrical and Computer Engineering - UFRN
Re: [PATCH, net] ibmvnic: fix firmware version when no firmware level has been provided by the VIOS server
Hello David,
Thank you for your review and the heads up about protocol.
On 02/01/2018 05:59 PM, David Miller wrote:
From: Desnes Augusto Nunes do Rosario
Date: Thu, 1 Feb 2018 16:04:30 -0200
Older versions of VIOS servers do not send the firmware level in the VPD
buffer for the ibmvnic driver. Thus, not only the current message is mis-
leading but the firmware version in the ethtool will be NULL. Therefore,
this patch fixes the firmware string and its warning.
Fixes: 4e6759be28e4 ("ibmvnic: Feature implementation of VPD for the ibmvnic
driver")
Signed-off-by: Desnes A. Nunes do Rosario
Applied.
Please do not put empty lines between Fixes: and Signed-off-by: and other
tags, all tags are equal and are placed together in an uninterrupted
sequences of consequetive lines.
Thank you.
--
Desnes Augusto Nunes do Rosário
--
Linux Developer - IBM / Brazil
M.Sc. in Electrical and Computer Engineering - UFRN
Re: [PATCH] [powerpc-next] Fix powerpc64 alignment of .toc section in kernel modules
Hello Michael,
On 12/07/2017 10:25 AM, Michael Ellerman wrote:
Hi Desnes,
Am I right that Alan largely wrote this patch?
If so it should probably be From: him, so that he is the author in the
git log.
Yes, Alan Modra is the main author and I am just committing it with
minor changes. Thus, the author change is necessary.
Desnes Augusto Nunes do Rosario writes:
diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
index 1381693..c472f5b 100644
--- a/arch/powerpc/Makefile
+++ b/arch/powerpc/Makefile
@@ -63,6 +63,7 @@ UTS_MACHINE := $(subst $(space),,$(machine-y))
ifdef CONFIG_PPC32
KBUILD_LDFLAGS_MODULE += arch/powerpc/lib/crtsavres.o
else
+KBUILD_LDFLAGS_MODULE += -T arch/powerpc/kernel/module.lds
This needs to be:
KBUILD_LDFLAGS_MODULE += -T $(srctree)/arch/powerpc/kernel/module.lds
Otherwise building with O=../build fails with:
ld: cannot open linker script file arch/powerpc/kernel/module.lds: No such
file or directory
I'll fix it up.
Indeed; this change is necessary to avoid any path errors.
diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index 759104b..9b2c5c1 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -374,11 +377,13 @@ int module_frob_arch_sections(Elf64_Ehdr *hdr,
}
/* r2 is the TOC pointer: it actually points 0x8000 into the TOC (this
- gives the value maximum span in an instruction which uses a signed
- offset) */
+ * gives the value maximum span in an instruction which uses a signed
+ * offset). Round down to a 256 byte boundary for the odd case where
+ * we are setting up r2 without a .toc section.
+ */
static inline unsigned long my_r2(const Elf64_Shdr *sechdrs, struct module
*me)
{
- return sechdrs[me->arch.toc_section].sh_addr + 0x8000;
+ return (sechdrs[me->arch.toc_section].sh_addr & -256) + 0x8000;
I think it's more typical in the kernel to write -256 as ~0xff.
Again I can fix it up.
Good to know!
cheers
Lastly, will you fix it up or do you want me to send a second version
then? Whatever is best for you.
Thank you for the review.
--
Desnes Augusto Nunes do Rosário
--
Linux Developer - IBM / Brazil
M.Sc. in Electrical and Computer Engineering - UFRN
(11) 9595-30-900
desn...@br.ibm.com
Re: [PATCH] [net-next,v2] ibmvnic: fix dma_mapping_error call
First of all, I apologize for sending this patch to net-next!
Since this is a fix, it should had been sent to the regular net tree,
which I'll do now with the proper fixes tag. My mistake!
Thanks for understanding and please discard this one.
On 11/16/2017 04:33 PM, Desnes Augusto Nunes do Rosario wrote:
This patch fixes the dma_mapping_error call to use the correct dma_addr
which is inside the ibmvnic_vpd struct. Moreover, it fixes a uninitialized
warning for the local dma_addr.
Signed-off-by: Desnes A. Nunes do Rosario
---
drivers/net/ethernet/ibm/ibmvnic.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/ibm/ibmvnic.c
b/drivers/net/ethernet/ibm/ibmvnic.c
index 04aaacb..1dc4aef 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -849,7 +849,6 @@ static int ibmvnic_get_vpd(struct ibmvnic_adapter *adapter)
{
struct device *dev = &adapter->vdev->dev;
union ibmvnic_crq crq;
- dma_addr_t dma_addr;
int len = 0;
if (adapter->vpd->buff)
@@ -879,7 +878,7 @@ static int ibmvnic_get_vpd(struct ibmvnic_adapter *adapter)
adapter->vpd->dma_addr =
dma_map_single(dev, adapter->vpd->buff, adapter->vpd->len,
DMA_FROM_DEVICE);
- if (dma_mapping_error(dev, dma_addr)) {
+ if (dma_mapping_error(dev, adapter->vpd->dma_addr)) {
dev_err(dev, "Could not map VPD buffer\n");
kfree(adapter->vpd->buff);
return -ENOMEM;
--
Desnes Augusto Nunes do Rosário
--
Linux Developer - IBM / Brazil
M.Sc. in Electrical and Computer Engineering - UFRN
(11) 9595-30-900
desn...@br.ibm.com
Re: [PATCH] [net-next] ibmvnic: This patch fixes the dma_mapping_error call to use the correct dma_addr which is inside the ibmvnic_vpd struct.
Version 2 of this patch has been already sent with correct styling.
On 11/16/2017 04:28 PM, Desnes Augusto Nunes do Rosario wrote:
Signed-off-by: Desnes A. Nunes do Rosario
---
drivers/net/ethernet/ibm/ibmvnic.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/ibm/ibmvnic.c
b/drivers/net/ethernet/ibm/ibmvnic.c
index 04aaacb..1dc4aef 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -849,7 +849,6 @@ static int ibmvnic_get_vpd(struct ibmvnic_adapter *adapter)
{
struct device *dev = &adapter->vdev->dev;
union ibmvnic_crq crq;
- dma_addr_t dma_addr;
int len = 0;
if (adapter->vpd->buff)
@@ -879,7 +878,7 @@ static int ibmvnic_get_vpd(struct ibmvnic_adapter *adapter)
adapter->vpd->dma_addr =
dma_map_single(dev, adapter->vpd->buff, adapter->vpd->len,
DMA_FROM_DEVICE);
- if (dma_mapping_error(dev, dma_addr)) {
+ if (dma_mapping_error(dev, adapter->vpd->dma_addr)) {
dev_err(dev, "Could not map VPD buffer\n");
kfree(adapter->vpd->buff);
return -ENOMEM;
--
Desnes Augusto Nunes do Rosário
--
Linux Developer - IBM / Brazil
M.Sc. in Electrical and Computer Engineering - UFRN
(11) 9595-30-900
desn...@br.ibm.com
Re: [PATCH] [net-next,v3] ibmvnic: Feature implementation of Vital Product Data (VPD) for the ibmvnic driver
On 11/10/2017 12:54 PM, Nathan Fontenot wrote:
On 11/10/2017 08:41 AM, Desnes Augusto Nunes do Rosário wrote:
On 11/09/2017 06:31 PM, Nathan Fontenot wrote:
On 11/09/2017 01:00 PM, Desnes Augusto Nunes do Rosario wrote:
This patch implements and enables VDP support for the ibmvnic driver.
Moreover, it includes the implementation of suitable structs, signal
transmission/handling and functions which allows the retrival of firmware
information from the ibmvnic card through the ethtool command.
Signed-off-by: Desnes A. Nunes do Rosario
Signed-off-by: Thomas Falcon
---
drivers/net/ethernet/ibm/ibmvnic.c | 149
-
drivers/net/ethernet/ibm/ibmvnic.h | 27 ++-
2 files changed, 173 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/ibm/ibmvnic.c
b/drivers/net/ethernet/ibm/ibmvnic.c
index d0cff28..693b502 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -573,6 +573,15 @@ static int reset_tx_pools(struct ibmvnic_adapter *adapter)
return 0;
}
+static void release_vpd_data(struct ibmvnic_adapter *adapter)
+{
+ if (!adapter->vpd)
+ return;
+
+ kfree(adapter->vpd->buff);
+ kfree(adapter->vpd);
+}
+
static void release_tx_pools(struct ibmvnic_adapter *adapter)
{
struct ibmvnic_tx_pool *tx_pool;
@@ -753,6 +762,8 @@ static void release_resources(struct ibmvnic_adapter
*adapter)
{
int i;
+ release_vpd_data(adapter);
+
release_tx_pools(adapter);
release_rx_pools(adapter);
@@ -833,6 +844,53 @@ static int set_real_num_queues(struct net_device *netdev)
return rc;
}
+static int ibmvnic_get_vpd(struct ibmvnic_adapter *adapter)
+{
+ struct device *dev = &adapter->vdev->dev;
+ union ibmvnic_crq crq;
+ dma_addr_t dma_addr;
+ int len;
+
+ if (adapter->vpd->buff)
+ len = adapter->vpd->len;
+
+ reinit_completion(&adapter->fw_done);
+ crq.get_vpd_size.first = IBMVNIC_CRQ_CMD;
+ crq.get_vpd_size.cmd = GET_VPD_SIZE;
+ ibmvnic_send_crq(adapter, &crq);
+ wait_for_completion(&adapter->fw_done);
+
Shouldn't there be a check for the return code when getting the
vpd size?
Hello Nathan,
This check is already being performed on the handle_vpd_size_rsp() function
down below.
In short, a GET_VPD_SIZE signal is sent here through a ibmvnic_crq union in
ibmvnic_send_crq(), whereas handle_query_ip_offload_rsp() receives from the VNIC
adapter a GET_VPD_SIZE_RSP containing a ibmvnic_crq union with the vpd size
information and the rc.code. If successful, a &adapter->fw_done is sent and
this part of the code continues; however if not, a dev_error() is thrown. Same logic
applies to GET_VPD/GET_VPD_RSP.
Yes, I did see that code. You do a complet of the completion variable for both
success and failure,
this then lets this routine continue irregardless of the results of the get vpd
size request. The
call to dev_err will print the error message but does not prevent use from
bailing if the
get vpd size fails. Perhaps setting vpd->len to -1 to indicate the get vpd call
failed which could
then be checked by the requester.
-Nathan
>> What I am adding on the next version of the patch is a check if
adapter->vpd->len is different than 0 before allocating
adapter->vpd->buff, since that in a case of a failure, adapter->vpd->len
will be 0.
I do concur with your observation that the break is necessary.
If the reception of vpd failed, adapter->vpd->len will be still zeroed
out since it was created with kzalloc in init_resources().
Thus, do you agree if in the next version I send the following code?
===
+ reinit_completion(&adapter->fw_done);
+ crq.get_vpd_size.first = IBMVNIC_CRQ_CMD;
+ crq.get_vpd_size.cmd = GET_VPD_SIZE;
+ ibmvnic_send_crq(adapter, &crq);
+ wait_for_completion(&adapter->fw_done);
+
->+ if(!adapter->vpd->len)
->+ return -ENODATA;
+
+ if (!adapter->vpd->buff)
+ adapter->vpd->buff = kzalloc(adapter->vpd->len,
GFP_KERNEL);
+ else if (adapter->vpd->len != len)
+ adapter->vpd->buff =
+ krealloc(adapter->vpd->buff,
+adapter->vpd->len, GFP_KERNEL);
===
Best Regards,
+ if (!adapter->vpd->buff)
+ adapter->vpd->buff = kzalloc(adapter->vpd->len, GFP_KERNEL);
+ else if (adapter->vpd->len != len)
+ adapter->vpd->buff =
+ krealloc(adapter->vpd->buff,
+ adapter->vpd->len, GFP_KERNEL);
+
+ if (!adapter->vpd->buff) {
+ dev_err(dev, "Could allocate VPD buffer\n");
+ return -ENOMEM;
+ }
+
+ adapter->vpd->dma_a
Re: [PATCH] [net-next,v3] ibmvnic: Feature implementation of Vital Product Data (VPD) for the ibmvnic driver
ed[4];
} __packed __aligned(8);
+struct ibmvnic_get_vpd_rsp {
+ u8 first;
+ u8 cmd;
+ u8 reserved[10];
+ struct ibmvnic_rc rc;
+} __packed __aligned(8);
+
struct ibmvnic_acl_change_indication {
u8 first;
u8 cmd;
@@ -700,10 +713,10 @@ union ibmvnic_crq {
struct ibmvnic_change_mac_addr change_mac_addr_rsp;
struct ibmvnic_multicast_ctrl multicast_ctrl;
struct ibmvnic_multicast_ctrl multicast_ctrl_rsp;
- struct ibmvnic_generic_crq get_vpd_size;
+ struct ibmvnic_get_vpd_size get_vpd_size;
struct ibmvnic_get_vpd_size_rsp get_vpd_size_rsp;
struct ibmvnic_get_vpd get_vpd;
- struct ibmvnic_generic_crq get_vpd_rsp;
+ struct ibmvnic_get_vpd_rsp get_vpd_rsp;
struct ibmvnic_acl_change_indication acl_change_indication;
struct ibmvnic_acl_query acl_query;
struct ibmvnic_generic_crq acl_query_rsp;
@@ -937,6 +950,12 @@ struct ibmvnic_error_buff {
__be32 error_id;
};
+struct ibmvnic_vpd {
+ unsigned char *buff;
+ dma_addr_t dma_addr;
+ u64 len;
+};
+
enum vnic_state {VNIC_PROBING = 1,
VNIC_PROBED,
VNIC_OPENING,
@@ -978,6 +997,10 @@ struct ibmvnic_adapter {
dma_addr_t ip_offload_ctrl_tok;
u32 msg_enable;
+ /* Vital Product Data (VPD) */
+ struct ibmvnic_vpd *vpd;
+ char fw_version[32];
+
/* Statistics */
struct ibmvnic_statistics stats;
dma_addr_t stats_token;
--
Desnes Augusto Nunes do Rosário
--
Linux Developer - IBM / Brazil
M.Sc. in Electrical and Computer Engineering - UFRN
desn...@br.ibm.com
