[PATCH] powerpc/pseries: fix of_read_drc_info_cell() to point at next record
The expectation is that when calling of_read_drc_info_cell()
repeatedly to parse multiple drc-info records that the in/out curval
parameter points at the start of the next record on return. However,
the current behavior has curval still pointing at the final value of
the record just parsed. The result of which is that if the
ibm,drc-info property contains multiple properties the parsed value
of the drc_type for any record after the first has the power_domain
value of the previous record appended to the type string.
Ex: observed the following 0x prepended to PHB
[ 69.485037] drc-info: type: \xff\xff\xff\xffPHB, prefix: PHB , index_start:
0x2001
[ 69.485038] drc-info: suffix_start: 1, sequential_elems: 3072,
sequential_inc: 1
[ 69.485038] drc-info: power-domain: 0x, last_index: 0x2c00
Fix by incrementing curval past the power_domain value to point at
drc_type string of next record.
Fixes: a29396653b8bf ("pseries/drc-info: Search DRC properties for CPU indexes")
Signed-off-by: Tyrel Datwyler
---
arch/powerpc/platforms/pseries/of_helpers.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/platforms/pseries/of_helpers.c
b/arch/powerpc/platforms/pseries/of_helpers.c
index 66dfd8256712..23241c71ef37 100644
--- a/arch/powerpc/platforms/pseries/of_helpers.c
+++ b/arch/powerpc/platforms/pseries/of_helpers.c
@@ -88,7 +88,7 @@ int of_read_drc_info_cell(struct property **prop, const
__be32 **curval,
return -EINVAL;
/* Should now know end of current entry */
- (*curval) = (void *)p2;
+ (*curval) = (void *)(++p2);
data->last_drc_index = data->drc_index_start +
((data->num_sequential_elems - 1) * data->sequential_inc);
--
2.16.4
Re: [PATCH 1/2] pseries/vio: Remove stray #ifdef CONFIG_PPC_PSERIES
On 1/29/20 10:31 PM, Oliver O'Halloran wrote: > vio.c requires CONFIG_IBMVIO which in turn depends on PPC_PSERIES. > In other words, this ifdef is pointless. At a guess it's a carry-over > from pre-history. > > Signed-off-by: Oliver O'Halloran Reviewed-by: Tyrel Datwyler
Re: [PATCH 2/2] pseries/makefile: Remove CONFIG_PPC_PSERIES check
On 1/29/20 10:31 PM, Oliver O'Halloran wrote: > The platform makefile (arch/powerpc/platforms/pseries/Makefile) is only > included by the platform makefile (arch/powerpc/platform/Makefile) when > CONFIG_PPC_PSERIES is selected, so checking for CONFIG_PPC_PSERIES in the > pseries makefile is pointless. > > Signed-off-by: Oliver O'Halloran Reviewed-by: Tyrel Datwyler
[PATCH] powerpc/pseries/vio: Fix iommu_table use-after-free refcount warning
From: Tyrel Datwyler
Commit e5afdf9dd515 ("powerpc/vfio_spapr_tce: Add reference counting to
iommu_table") missed an iommu_table allocation in the pseries vio code.
The iommu_table is allocated with kzalloc and as a result the associated
kref gets a value of zero. This has the side effect that during a DLPAR
remove of the associated virtual IOA the iommu_tce_table_put() triggers
a use-after-free underflow warning.
Call Trace:
[c002879e39f0] [c071ecb4] refcount_warn_saturate+0x184/0x190
(unreliable)
[c002879e3a50] [c00500ac] iommu_tce_table_put+0x9c/0xb0
[c002879e3a70] [c00f54e4] vio_dev_release+0x34/0x70
[c002879e3aa0] [c087cfa4] device_release+0x54/0xf0
[c002879e3b10] [c0d64c84] kobject_cleanup+0xa4/0x240
[c002879e3b90] [c087d358] put_device+0x28/0x40
[c002879e3bb0] [c07a328c] dlpar_remove_slot+0x15c/0x250
[c002879e3c50] [c07a348c] remove_slot_store+0xac/0xf0
[c002879e3cd0] [c0d64220] kobj_attr_store+0x30/0x60
[c002879e3cf0] [c04ff13c] sysfs_kf_write+0x6c/0xa0
[c002879e3d10] [c04fde4c] kernfs_fop_write+0x18c/0x260
[c002879e3d60] [c0410f3c] __vfs_write+0x3c/0x70
[c002879e3d80] [c0415408] vfs_write+0xc8/0x250
[c002879e3dd0] [c04157dc] ksys_write+0x7c/0x120
[c002879e3e20] [c000b278] system_call+0x5c/0x68
Further, since the refcount was always zero the iommu_tce_table_put()
fails to call the iommu_table release function resulting in a leak.
Fix this issue be initilizing the iommu_table kref immediately after
allocation.
Fixes: e5afdf9dd515 ("powerpc/vfio_spapr_tce: Add reference counting to
iommu_table")
Signed-off-by: Tyrel Datwyler
---
arch/powerpc/platforms/pseries/vio.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/powerpc/platforms/pseries/vio.c
b/arch/powerpc/platforms/pseries/vio.c
index 79e2287..f682b7b 100644
--- a/arch/powerpc/platforms/pseries/vio.c
+++ b/arch/powerpc/platforms/pseries/vio.c
@@ -1176,6 +1176,8 @@ static struct iommu_table *vio_build_iommu_table(struct
vio_dev *dev)
if (tbl == NULL)
return NULL;
+ kref_init(&tbl->it_kref);
+
of_parse_dma_window(dev->dev.of_node, dma_window,
&tbl->it_index, &offset, &size);
--
1.8.3.1
Re: [PATCH v2] Fix display of Maximum Memory
On 1/15/20 6:53 AM, Michael Bringmann wrote:
> Correct overflow problem in calculation+display of Maximum Memory
> value to syscfg where 32bits is insufficient.
>
Probably needs the following Fixes tag:
Fixes: 772b039fd9a7 ("powerpc/pseries: Export maximum memory value")
otherwise,
Reviewed-by: Tyrel Datwyler
> Signed-off-by: Michael Bringmann
> ---
> arch/powerpc/platforms/pseries/lparcfg.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/platforms/pseries/lparcfg.c
> b/arch/powerpc/platforms/pseries/lparcfg.c
> index e33e8bc..f00411c 100644
> --- a/arch/powerpc/platforms/pseries/lparcfg.c
> +++ b/arch/powerpc/platforms/pseries/lparcfg.c
> @@ -433,12 +433,12 @@ static void parse_em_data(struct seq_file *m)
>
> static void maxmem_data(struct seq_file *m)
> {
> - unsigned long maxmem = 0;
> + u64 maxmem = 0;
>
> - maxmem += drmem_info->n_lmbs * drmem_info->lmb_size;
> - maxmem += hugetlb_total_pages() * PAGE_SIZE;
> + maxmem += (u64)drmem_info->n_lmbs * drmem_info->lmb_size;
> + maxmem += (u64)hugetlb_total_pages() * PAGE_SIZE;
>
> - seq_printf(m, "MaxMem=%ld\n", maxmem);
> + seq_printf(m, "MaxMem=%llu\n", maxmem);
> }
>
> static int pseries_lparcfg_data(struct seq_file *m, void *v)
>
Re: [PATCH] powerpc/pseries: remove variable 'status' set but not used
On 11/18/19 9:53 PM, Michael Ellerman wrote:
> Chen Wandun writes:
>> Fixes gcc '-Wunused-but-set-variable' warning:
>>
>> arch/powerpc/platforms/pseries/ras.c: In function ras_epow_interrupt:
>> arch/powerpc/platforms/pseries/ras.c:319:6: warning: variable status set but
>> not used [-Wunused-but-set-variable]
>
> Thanks for the patch.
>
> But it almost certainly is wrong to not check the status.
Agreed, I started drafting a NACK response, but got sidetracked.
>
> It's calling firmware and just assuming that the call succeeded. It then
> goes on to use the result that should have been written by firmware, but
> is now potentially random junk.
>
> So I'd much rather a patch to change it to check the status.
+1
>
>> diff --git a/arch/powerpc/platforms/pseries/ras.c
>> b/arch/powerpc/platforms/pseries/ras.c
>> index 1d7f973..4a61d0f 100644
>> --- a/arch/powerpc/platforms/pseries/ras.c
>> +++ b/arch/powerpc/platforms/pseries/ras.c
>> @@ -316,12 +316,11 @@ static irqreturn_t ras_hotplug_interrupt(int irq, void
>> *dev_id)
>> /* Handle environmental and power warning (EPOW) interrupts. */
>> static irqreturn_t ras_epow_interrupt(int irq, void *dev_id)
>> {
>> -int status;
>> int state;
>> int critical;
>>
>> -status = rtas_get_sensor_fast(EPOW_SENSOR_TOKEN, EPOW_SENSOR_INDEX,
>> - &state);
>> +rtas_get_sensor_fast(EPOW_SENSOR_TOKEN, EPOW_SENSOR_INDEX,
>> + &state);
>
> This is calling a helper which already does some translation of the
> return value, any value < 0 indicates an error.
There are three possible architected failures here: Hardware, Non-existant
sensor, and an DR isolation error which namely would be reported in the status
as -EIO, -EINVAL, and -EFAULT. Further, the EPOW sensor is required, and is not
a DR entity so we can never get an -EINVAL or -EFAULT (baring broken firmware).
This leaves -EIO (HARDWARE_ERROR) and as I mention further down this will
generate its own error log in response. So, I don't think we need to do any
reporting here, and just return.
>
>> @@ -330,12 +329,12 @@ static irqreturn_t ras_epow_interrupt(int irq, void
>> *dev_id)
>>
>> spin_lock(&ras_log_buf_lock);
>>
>> -status = rtas_call(ras_check_exception_token, 6, 1, NULL,
>> - RTAS_VECTOR_EXTERNAL_INTERRUPT,
>> - virq_to_hw(irq),
>> - RTAS_EPOW_WARNING,
>> - critical, __pa(&ras_log_buf),
>> -rtas_get_error_log_max());
>> +rtas_call(ras_check_exception_token, 6, 1, NULL,
>> + RTAS_VECTOR_EXTERNAL_INTERRUPT,
>> + virq_to_hw(irq),
>> + RTAS_EPOW_WARNING,
>> + critical, __pa(&ras_log_buf),
>> + rtas_get_error_log_max());
>
> This is directly calling firmware.
>
> As documented in LoPAPR, a negative status indicates an error, 0
> indicates a new error log was found (ie. the function should continue),
> or 1 there was no error log (ie. nothing to do).
It is highly unlikely that we will find no new error log since we are processing
an interrupt that supposedly fired to tell us there is a new one. However, the
ras_log_buf is never zeroed so in the unlikely case there is no new error log we
will parse stale data from the previous log. Better safe than sorry and just
return.
In the case of an error the only error code we supposedly can get here is -1
(HARDWARE_ERROR), and the RTAS handling will generate an error log in response
to that. So, I don't think we need to report anything here. I would suggest for
the (status != 0) case that you just return.
-Tyrel
>
> cheers
>
>> log_error(ras_log_buf, ERR_TYPE_RTAS_LOG, 0);
>>
>> --
>> 2.7.4
Re: [PATCH] of: unittest: fix memory leak in attach_node_and_children
On 11/14/19 10:43 AM, Erhard Furtner wrote:
> In attach_node_and_children memory is allocated for full_name via
> kasprintf. If the condition of the 1st if is not met the function
> returns early without freeing the memory. Add a kfree() to fix that.
>
> Signed-off-by: Erhard Furtner
Michael provided instructions about what needs to be done with a v2 spin of your
patch to get it upstream. Please feel free to include my reviewed-by as well.
Reviewed-by: Tyrel Datwyler
> ---
> drivers/of/unittest.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
> index 92e895d86458..ca7823eef2b4 100644
> --- a/drivers/of/unittest.c
> +++ b/drivers/of/unittest.c
> @@ -1146,8 +1146,10 @@ static void attach_node_and_children(struct
> device_node *np)
> full_name = kasprintf(GFP_KERNEL, "%pOF", np);
>
> if (!strcmp(full_name, "/__local_fixups__") ||
> - !strcmp(full_name, "/__fixups__"))
> + !strcmp(full_name, "/__fixups__")) {
> + kfree(full_name);
> return;
> + }
>
> dup = of_find_node_by_path(full_name);
> kfree(full_name);
>
Re: [PATCH] powerpc/powernv: Disable native PCIe port management
Nothing but pedantic spelling and grammar nits of the commit log follow.
-Tyrel
On 11/13/19 1:40 AM, Oliver O'Halloran wrote:
> On PowerNV the PCIe topology is (currently) managed the powernv platform
s/the powernv/by the powernv
> code in cooperation with firmware. The PCIe-native service drivers bypass
> both and this can cause problems.
>
> Historically this hasn't been a big deal since the only port service
> driver that saw much use was the AER driver. The AER driver relies
> a kernel service to report when errors occur rather than acting autonmously
s/a kernel/on a kernel
autonomously
> so it's fairly easy to ignore. On PowerNV (and pseries) AER events are
> handled through EEH, which ignores the AER service, so it's never been
> an issue.
>
> Unfortunately, the hotplug port service driver (pciehp) does act
> autonomously and conflicts with the platform specific hotplug
> driver (pnv_php). The main issue is that pciehp claims the interrupt
> associated with the PCIe capability which in turn prevents pnv_php from
> claiming it.
>
> This results in hotplug events being handled by pciehp which does not
> notify firmware when the PCIe topology changes, and does not setup/teardown
> the arch specific PCI device structures (pci_dn) when the topology changes.
> The end result is that hot-added devices cannot be enabled and hot-removed
> devices may not be fully torn-down on removal.
>
> We can fix these problems by setting the "pcie_ports_disabled" flag during
> platform initialisation. The flag indicates the platform owns the PCIe
> ports which stops the portbus driver being registered.
s/being/from being
>
> Cc: Sergey Miroshnichenko
> Fixes: 66725152fb9f ("PCI/hotplug: PowerPC PowerNV PCI hotplug driver")
> Signed-off-by: Oliver O'Halloran
> ---
> Sergey, just FYI. I'll try sort out the rest of the hotplug
> trainwreck in 5.6.
>
> The Fixes: here is for the patch that added pnv_php in 4.8. It's been
> a problem since then, but wasn't noticed until people started testing
> it after the EEH fixes in commit 799abe283e51 ("powerpc/eeh: Clean up
> EEH PEs after recovery finishes") went in earlier in the 5.4 cycle.
> ---
> arch/powerpc/platforms/powernv/pci.c | 17 +
> 1 file changed, 17 insertions(+)
>
> diff --git a/arch/powerpc/platforms/powernv/pci.c
> b/arch/powerpc/platforms/powernv/pci.c
> index 2825d00..ae62583 100644
> --- a/arch/powerpc/platforms/powernv/pci.c
> +++ b/arch/powerpc/platforms/powernv/pci.c
> @@ -941,6 +941,23 @@ void __init pnv_pci_init(void)
>
> pci_add_flags(PCI_CAN_SKIP_ISA_ALIGN);
>
> +#ifdef CONFIG_PCIEPORTBUS
> + /*
> + * On PowerNV PCIe devices are (currently) managed in cooperation
> + * with firmware. This isn't *strictly* required, but there's enough
> + * assumptions baked into both firmware and the platform code that
> + * it's unwise to allow the portbus services to be used.
> + *
> + * We need to fix this eventually, but for now set this flag to disable
> + * the portbus driver. The AER service isn't required since that AER
> + * events are handled via EEH. The pciehp hotplug driver can't work
> + * without kernel changes (and portbus binding breaks pnv_php). The
> + * other services also require some thinking about how we're going
> + * to integrate them.
> + */
> + pcie_ports_disabled = true;
> +#endif
> +
> /* If we don't have OPAL, eg. in sim, just skip PCI probe */
> if (!firmware_has_feature(FW_FEATURE_OPAL))
> return;
>
[PATCH v2 7/9] PCI: rpaphp: annotate and correctly byte swap DRC properties
The device tree is in big endian format and any properties directly
retrieved using OF helpers that don't explicitly byte swap should
be annotated. In particular there are several places where we grab
the opaque property value for the old ibm,drc-* properties and the
ibm,my-drc-index property.
Fix this for better static checking by annotating values we know to
explicitly big endian, and byte swap where appropriate.
Signed-off-by: Tyrel Datwyler
---
drivers/pci/hotplug/rpaphp_core.c | 29 +++--
1 file changed, 15 insertions(+), 14 deletions(-)
diff --git a/drivers/pci/hotplug/rpaphp_core.c
b/drivers/pci/hotplug/rpaphp_core.c
index 75d5771..129534c 100644
--- a/drivers/pci/hotplug/rpaphp_core.c
+++ b/drivers/pci/hotplug/rpaphp_core.c
@@ -154,11 +154,11 @@ static enum pci_bus_speed get_max_bus_speed(struct slot
*slot)
return speed;
}
-static int get_children_props(struct device_node *dn, const int **drc_indexes,
- const int **drc_names, const int **drc_types,
- const int **drc_power_domains)
+static int get_children_props(struct device_node *dn, const __be32
**drc_indexes,
+ const __be32 **drc_names, const __be32
**drc_types,
+ const __be32 **drc_power_domains)
{
- const int *indexes, *names, *types, *domains;
+ const __be32 *indexes, *names, *types, *domains;
indexes = of_get_property(dn, "ibm,drc-indexes", NULL);
names = of_get_property(dn, "ibm,drc-names", NULL);
@@ -194,8 +194,8 @@ static int rpaphp_check_drc_props_v1(struct device_node
*dn, char *drc_name,
char *drc_type, unsigned int my_index)
{
char *name_tmp, *type_tmp;
- const int *indexes, *names;
- const int *types, *domains;
+ const __be32 *indexes, *names;
+ const __be32 *types, *domains;
int i, rc;
rc = get_children_props(dn->parent, &indexes, &names, &types, &domains);
@@ -208,7 +208,7 @@ static int rpaphp_check_drc_props_v1(struct device_node
*dn, char *drc_name,
/* Iterate through parent properties, looking for my-drc-index */
for (i = 0; i < be32_to_cpu(indexes[0]); i++) {
- if ((unsigned int) indexes[i + 1] == my_index)
+ if (be32_to_cpu(indexes[i + 1]) == my_index)
break;
name_tmp += (strlen(name_tmp) + 1);
@@ -267,7 +267,7 @@ static int rpaphp_check_drc_props_v2(struct device_node
*dn, char *drc_name,
int rpaphp_check_drc_props(struct device_node *dn, char *drc_name,
char *drc_type)
{
- const unsigned int *my_index;
+ const __be32 *my_index;
my_index = of_get_property(dn, "ibm,my-drc-index", NULL);
if (!my_index) {
@@ -277,10 +277,10 @@ int rpaphp_check_drc_props(struct device_node *dn, char
*drc_name,
if (of_find_property(dn->parent, "ibm,drc-info", NULL))
return rpaphp_check_drc_props_v2(dn, drc_name, drc_type,
- *my_index);
+ be32_to_cpu(*my_index));
else
return rpaphp_check_drc_props_v1(dn, drc_name, drc_type,
- *my_index);
+ be32_to_cpu(*my_index));
}
EXPORT_SYMBOL_GPL(rpaphp_check_drc_props);
@@ -311,10 +311,11 @@ static int is_php_type(char *drc_type)
* for built-in pci slots (even when the built-in slots are
* dlparable.)
*/
-static int is_php_dn(struct device_node *dn, const int **indexes,
- const int **names, const int **types, const int **power_domains)
+static int is_php_dn(struct device_node *dn, const __be32 **indexes,
+const __be32 **names, const __be32 **types,
+const __be32 **power_domains)
{
- const int *drc_types;
+ const __be32 *drc_types;
int rc;
rc = get_children_props(dn, indexes, names, &drc_types, power_domains);
@@ -374,7 +375,7 @@ static int rpaphp_drc_add_slot(struct device_node *dn)
struct slot *slot;
int retval = 0;
int i;
- const int *indexes, *names, *types, *power_domains;
+ const __be32 *indexes, *names, *types, *power_domains;
char *name, *type;
/* If this is not a hotplug slot, return without doing anything. */
--
2.7.4
[PATCH v2 3/9] powerpc/pseries: Add cpu DLPAR support for drc-info property
Older firmwares provided information about Dynamic Reconfig
Connectors (DRC) through several device tree properties, namely
ibm,drc-types, ibm,drc-indexes, ibm,drc-names, and
ibm,drc-power-domains. New firmwares have the ability to present this
same information in a much condensed format through a device tree
property called ibm,drc-info.
The existing cpu DLPAR hotplug code only understands the older DRC
property format when validating the drc-index of a cpu during a
hotplug add. This updates those code paths to use the ibm,drc-info
property, when present, instead for validation.
Signed-off-by: Tyrel Datwyler
---
arch/powerpc/platforms/pseries/hotplug-cpu.c | 127 +++
1 file changed, 112 insertions(+), 15 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c
b/arch/powerpc/platforms/pseries/hotplug-cpu.c
index bbda646..967c5e1 100644
--- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
+++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
@@ -407,17 +407,67 @@ static bool dlpar_cpu_exists(struct device_node *parent,
u32 drc_index)
return found;
}
+static bool drc_info_valid_index(struct device_node *parent, u32 drc_index)
+{
+ struct property *info;
+ struct of_drc_info drc;
+ const __be32 *value;
+ u32 index;
+ int count, i, j;
+
+ info = of_find_property(parent, "ibm,drc-info", NULL);
+ if (!info)
+ return false;
+
+ value = of_prop_next_u32(info, NULL, &count);
+
+ /* First value of ibm,drc-info is number of drc-info records */
+ if (value)
+ value++;
+ else
+ return false;
+
+ for (i = 0; i < count; i++) {
+ if (of_read_drc_info_cell(&info, &value, &drc))
+ return false;
+
+ if (strncmp(drc.drc_type, "CPU", 3))
+ break;
+
+ if (drc_index > drc.last_drc_index)
+ continue;
+
+ index = drc.drc_index_start;
+ for (j = 0; j < drc.num_sequential_elems; j++) {
+ if (drc_index == index)
+ return true;
+
+ index += drc.sequential_inc;
+ }
+ }
+
+ return false;
+}
+
static bool valid_cpu_drc_index(struct device_node *parent, u32 drc_index)
{
bool found = false;
int rc, index;
- index = 0;
+ if (of_find_property(parent, "ibm,drc-info", NULL))
+ return drc_info_valid_index(parent, drc_index);
+
+ /* Note that the format of the ibm,drc-indexes array is
+* the number of entries in the array followed by the array
+* of drc values so we start looking at index = 1.
+*/
+ index = 1;
while (!found) {
u32 drc;
rc = of_property_read_u32_index(parent, "ibm,drc-indexes",
index++, &drc);
+
if (rc)
break;
@@ -717,19 +767,52 @@ static int dlpar_cpu_remove_by_count(u32 cpus_to_remove)
return rc;
}
-static int find_dlpar_cpus_to_add(u32 *cpu_drcs, u32 cpus_to_add)
+static int find_drc_info_cpus_to_add(struct device_node *cpus,
+struct property *info,
+u32 *cpu_drcs, u32 cpus_to_add)
{
- struct device_node *parent;
+ struct of_drc_info drc;
+ const __be32 *value;
+ u32 count, drc_index;
int cpus_found = 0;
- int index, rc;
+ int i, j;
- parent = of_find_node_by_path("/cpus");
- if (!parent) {
- pr_warn("Could not find CPU root node in device tree\n");
- kfree(cpu_drcs);
+ if (!info)
return -1;
+
+ value = of_prop_next_u32(info, NULL, &count);
+ if (value)
+ value++;
+
+ for (i = 0; i < count; i++) {
+ of_read_drc_info_cell(&info, &value, &drc);
+ if (strncmp(drc.drc_type, "CPU", 3))
+ break;
+
+ drc_index = drc.drc_index_start;
+ for (j = 0; j < drc.num_sequential_elems; j++) {
+ if (dlpar_cpu_exists(cpus, drc_index))
+ continue;
+
+ cpu_drcs[cpus_found++] = drc_index;
+
+ if (cpus_found == cpus_to_add)
+ return cpus_found;
+
+ drc_index += drc.sequential_inc;
+ }
}
+ return cpus_found;
+}
+
+static int find_drc_index_cpus_to_add(struct device_node *cpus,
+ u32 *cpu_drcs, u32 cpus_to_add)
+{
+ int cpus_found = 0;
+ int index, rc;
+ u32 drc_index;
+
/* Search the ibm,drc-i
[PATCH v2 8/9] PCI: rpaphp: Correctly match ibm, my-drc-index to drc-name when using drc-info
The newer ibm,drc-info property is a condensed description of the old
ibm,drc-* properties (ie. names, types, indexes, and power-domains).
When matching a drc-index to a drc-name we need to verify that the
index is within the start and last drc-index range and map it to a
drc-name using the drc-name-prefix and logical index.
Fix the mapping by checking that the index is within the range of the
current drc-info entry, and build the name from the drc-name-prefix
concatenated with the starting drc-name-suffix value and the sequential
index obtained by subtracting ibm,my-drc-index from this entries
drc-start-index.
Signed-off-by: Tyrel Datwyler
---
drivers/pci/hotplug/rpaphp_core.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/hotplug/rpaphp_core.c
b/drivers/pci/hotplug/rpaphp_core.c
index 129534c..951f7f2 100644
--- a/drivers/pci/hotplug/rpaphp_core.c
+++ b/drivers/pci/hotplug/rpaphp_core.c
@@ -248,9 +248,10 @@ static int rpaphp_check_drc_props_v2(struct device_node
*dn, char *drc_name,
/* Should now know end of current entry */
/* Found it */
- if (my_index <= drc.last_drc_index) {
+ if (my_index >= drc.drc_index_start && my_index <=
drc.last_drc_index) {
+ int index = my_index - drc.drc_index_start;
sprintf(cell_drc_name, "%s%d", drc.drc_name_prefix,
- my_index);
+ drc.drc_name_suffix_start + index);
break;
}
}
--
2.7.4
[PATCH v2 2/9] powerpc/pseries: Fix drc-info mappings of logical cpus to drc-index
There are a couple subtle errors in the mapping between cpu-ids and a
cpus associated drc-index when using the new ibm,drc-info property.
The first is that while drc-info may have been a supported firmware
feature at boot it is possible we have migrated to a CEC with older
firmware that doesn't support the ibm,drc-info property. In that case
the device tree would have been updated after migration to remove the
ibm,drc-info property and replace it with the older style ibm,drc-*
properties for types, indexes, names, and power-domains. PAPR even
goes as far as dictating that if we advertise support for drc-info
that we are capable of supporting either property type at runtime.
The second is that the first value of the ibm,drc-info property is
the int encoded count of drc-info entries. As such "value" returned
by of_prop_next_u32() is pointing at that count, and not the first
element of the first drc-info entry as is expected by the
of_read_drc_info_cell() helper.
Fix the first by ignoring DRC-INFO firmware feature and instead
testing directly for ibm,drc-info, and then falling back to the
old style ibm,drc-indexes in the case it doesn't exit.
Fix the second by incrementing value to the next element prior to
parsing drc-info entries.
Signed-off-by: Tyrel Datwyler
---
arch/powerpc/platforms/pseries/pseries_energy.c | 23 ++-
1 file changed, 10 insertions(+), 13 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/pseries_energy.c
b/arch/powerpc/platforms/pseries/pseries_energy.c
index a96874f..09e98d3 100644
--- a/arch/powerpc/platforms/pseries/pseries_energy.c
+++ b/arch/powerpc/platforms/pseries/pseries_energy.c
@@ -36,6 +36,7 @@ static int sysfs_entries;
static u32 cpu_to_drc_index(int cpu)
{
struct device_node *dn = NULL;
+ struct property *info;
int thread_index;
int rc = 1;
u32 ret = 0;
@@ -47,20 +48,18 @@ static u32 cpu_to_drc_index(int cpu)
/* Convert logical cpu number to core number */
thread_index = cpu_core_index_of_thread(cpu);
- if (firmware_has_feature(FW_FEATURE_DRC_INFO)) {
- struct property *info = NULL;
+ info = of_find_property(dn, "ibm,drc-info", NULL);
+ if (info) {
struct of_drc_info drc;
int j;
u32 num_set_entries;
const __be32 *value;
- info = of_find_property(dn, "ibm,drc-info", NULL);
- if (info == NULL)
- goto err_of_node_put;
-
value = of_prop_next_u32(info, NULL, &num_set_entries);
if (!value)
goto err_of_node_put;
+ else
+ value++;
for (j = 0; j < num_set_entries; j++) {
@@ -110,6 +109,7 @@ static u32 cpu_to_drc_index(int cpu)
static int drc_index_to_cpu(u32 drc_index)
{
struct device_node *dn = NULL;
+ struct property *info;
const int *indexes;
int thread_index = 0, cpu = 0;
int rc = 1;
@@ -117,21 +117,18 @@ static int drc_index_to_cpu(u32 drc_index)
dn = of_find_node_by_path("/cpus");
if (dn == NULL)
goto err;
-
- if (firmware_has_feature(FW_FEATURE_DRC_INFO)) {
- struct property *info = NULL;
+ info = of_find_property(dn, "ibm,drc-info", NULL);
+ if (info) {
struct of_drc_info drc;
int j;
u32 num_set_entries;
const __be32 *value;
- info = of_find_property(dn, "ibm,drc-info", NULL);
- if (info == NULL)
- goto err_of_node_put;
-
value = of_prop_next_u32(info, NULL, &num_set_entries);
if (!value)
goto err_of_node_put;
+ else
+ value++;
for (j = 0; j < num_set_entries; j++) {
--
2.7.4
[PATCH v2 1/9] powerpc/pseries: Fix bad drc_index_start value parsing of drc-info entry
The ibm,drc-info property is an array property that contains drc-info
entries such that each entry is made up of 2 string encoded elements
followed by 5 int encoded elements. The of_read_drc_info_cell()
helper contains comments that correctly name the expected elements
and their encoding. However, the usage of of_prop_next_string() and
of_prop_next_u32() introduced a subtle skippage of the first u32.
This is a result of of_prop_next_string() returning a pointer to the
next property value which is not a string, but actually a (__be32 *).
As, a result the following call to of_prop_next_u32() passes over the
current int encoded value and actually stores the next one wrongly.
Simply endian swap the current value in place after reading the first
two string values. The remaining int encoded values can then be read
correctly using of_prop_next_u32().
Signed-off-by: Tyrel Datwyler
---
arch/powerpc/platforms/pseries/of_helpers.c | 8 +++-
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/of_helpers.c
b/arch/powerpc/platforms/pseries/of_helpers.c
index 6df192f..66dfd82 100644
--- a/arch/powerpc/platforms/pseries/of_helpers.c
+++ b/arch/powerpc/platforms/pseries/of_helpers.c
@@ -45,14 +45,14 @@ struct device_node *pseries_of_derive_parent(const char
*path)
int of_read_drc_info_cell(struct property **prop, const __be32 **curval,
struct of_drc_info *data)
{
- const char *p;
+ const char *p = (char *)(*curval);
const __be32 *p2;
if (!data)
return -EINVAL;
/* Get drc-type:encode-string */
- p = data->drc_type = (char*) (*curval);
+ data->drc_type = (char *)p;
p = of_prop_next_string(*prop, p);
if (!p)
return -EINVAL;
@@ -65,9 +65,7 @@ int of_read_drc_info_cell(struct property **prop, const
__be32 **curval,
/* Get drc-index-start:encode-int */
p2 = (const __be32 *)p;
- p2 = of_prop_next_u32(*prop, p2, &data->drc_index_start);
- if (!p2)
- return -EINVAL;
+ data->drc_index_start = be32_to_cpu(*p2);
/* Get drc-name-suffix-start:encode-int */
p2 = of_prop_next_u32(*prop, p2, &data->drc_name_suffix_start);
--
2.7.4
[PATCH v2 9/9] powerpc/pseries: Enable support for ibm, drc-info property
Advertise client support for the PAPR architected ibm,drc-info device
tree property during CAS handshake.
Fixes: c7a3275e0f9e ("powerpc/pseries: Revert support for ibm,drc-info devtree
property")
Signed-off-by: Tyrel Datwyler
---
arch/powerpc/kernel/prom_init.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
index 100f1b5..eba9d4e 100644
--- a/arch/powerpc/kernel/prom_init.c
+++ b/arch/powerpc/kernel/prom_init.c
@@ -1053,7 +1053,7 @@ static const struct ibm_arch_vec
ibm_architecture_vec_template __initconst = {
.reserved2 = 0,
.reserved3 = 0,
.subprocessors = 1,
- .byte22 = OV5_FEAT(OV5_DRMEM_V2),
+ .byte22 = OV5_FEAT(OV5_DRMEM_V2) | OV5_FEAT(OV5_DRC_INFO),
.intarch = 0,
.mmu = 0,
.hash_ext = 0,
--
2.7.4
[PATCH v2 5/9] PCI: rpaphp: Don't rely on firmware feature to imply drc-info support
In the event that the partition is migrated to a platform with older firmware that doesn't support the ibm,drc-info property the device tree is modified to remove the ibm,drc-info property and replace it with the older style ibm,drc-* properties for types, names, indexes, and power-domains. One of the requirements of the drc-info firmware feature is that the client is able to handle both the new property, and old style properties at runtime. Therefore we can't rely on the firmware feature alone to dictate which property is currently present in the device tree. Fix this short coming by checking explicitly for the ibm,drc-info property, and falling back to the older ibm,drc-* properties if it doesn't exist. Signed-off-by: Tyrel Datwyler --- drivers/pci/hotplug/rpaphp_core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/pci/hotplug/rpaphp_core.c b/drivers/pci/hotplug/rpaphp_core.c index e350264..e18e9a0 100644 --- a/drivers/pci/hotplug/rpaphp_core.c +++ b/drivers/pci/hotplug/rpaphp_core.c @@ -275,7 +275,7 @@ int rpaphp_check_drc_props(struct device_node *dn, char *drc_name, return -EINVAL; } - if (firmware_has_feature(FW_FEATURE_DRC_INFO)) + if (of_find_property(dn->parent, "ibm,drc-info", NULL)) return rpaphp_check_drc_props_v2(dn, drc_name, drc_type, *my_index); else -- 2.7.4
[PATCH v2 6/9] PCI: rpaphp: Add drc-info support for hotplug slot registration
Split physical PCI slot registration scanning into separate routines
that support the old ibm,drc-* properties and one that supports the
new compressed ibm,drc-info property.
Signed-off-by: Tyrel Datwyler
---
drivers/pci/hotplug/rpaphp_core.c | 89 ++-
1 file changed, 69 insertions(+), 20 deletions(-)
diff --git a/drivers/pci/hotplug/rpaphp_core.c
b/drivers/pci/hotplug/rpaphp_core.c
index e18e9a0..75d5771 100644
--- a/drivers/pci/hotplug/rpaphp_core.c
+++ b/drivers/pci/hotplug/rpaphp_core.c
@@ -328,23 +328,48 @@ static int is_php_dn(struct device_node *dn, const int
**indexes,
return 1;
}
-/**
- * rpaphp_add_slot -- declare a hotplug slot to the hotplug subsystem.
- * @dn: device node of slot
- *
- * This subroutine will register a hotpluggable slot with the
- * PCI hotplug infrastructure. This routine is typically called
- * during boot time, if the hotplug slots are present at boot time,
- * or is called later, by the dlpar add code, if the slot is
- * being dynamically added during runtime.
- *
- * If the device node points at an embedded (built-in) slot, this
- * routine will just return without doing anything, since embedded
- * slots cannot be hotplugged.
- *
- * To remove a slot, it suffices to call rpaphp_deregister_slot().
- */
-int rpaphp_add_slot(struct device_node *dn)
+static int rpaphp_drc_info_add_slot(struct device_node *dn)
+{
+ struct slot *slot;
+ struct property *info;
+ struct of_drc_info drc;
+ char drc_name[MAX_DRC_NAME_LEN];
+ const __be32 *cur;
+ u32 count;
+ int retval = 0;
+
+ info = of_find_property(dn, "ibm,drc-info", NULL);
+ if (!info)
+ return 0;
+
+ cur = of_prop_next_u32(info, NULL, &count);
+ if (cur)
+ cur++;
+ else
+ return 0;
+
+ of_read_drc_info_cell(&info, &cur, &drc);
+ if (!is_php_type(drc.drc_type))
+ return 0;
+
+ sprintf(drc_name, "%s%d", drc.drc_name_prefix,
drc.drc_name_suffix_start);
+
+ slot = alloc_slot_struct(dn, drc.drc_index_start, drc_name,
drc.drc_power_domain);
+ if (!slot)
+ return -ENOMEM;
+
+ slot->type = simple_strtoul(drc.drc_type, NULL, 10);
+ retval = rpaphp_enable_slot(slot);
+ if (!retval)
+ retval = rpaphp_register_slot(slot);
+
+ if (retval)
+ dealloc_slot_struct(slot);
+
+ return retval;
+}
+
+static int rpaphp_drc_add_slot(struct device_node *dn)
{
struct slot *slot;
int retval = 0;
@@ -352,9 +377,6 @@ int rpaphp_add_slot(struct device_node *dn)
const int *indexes, *names, *types, *power_domains;
char *name, *type;
- if (!dn->name || strcmp(dn->name, "pci"))
- return 0;
-
/* If this is not a hotplug slot, return without doing anything. */
if (!is_php_dn(dn, &indexes, &names, &types, &power_domains))
return 0;
@@ -393,6 +415,33 @@ int rpaphp_add_slot(struct device_node *dn)
/* XXX FIXME: reports a failure only if last entry in loop failed */
return retval;
}
+
+/**
+ * rpaphp_add_slot -- declare a hotplug slot to the hotplug subsystem.
+ * @dn: device node of slot
+ *
+ * This subroutine will register a hotpluggable slot with the
+ * PCI hotplug infrastructure. This routine is typically called
+ * during boot time, if the hotplug slots are present at boot time,
+ * or is called later, by the dlpar add code, if the slot is
+ * being dynamically added during runtime.
+ *
+ * If the device node points at an embedded (built-in) slot, this
+ * routine will just return without doing anything, since embedded
+ * slots cannot be hotplugged.
+ *
+ * To remove a slot, it suffices to call rpaphp_deregister_slot().
+ */
+int rpaphp_add_slot(struct device_node *dn)
+{
+ if (!dn->name || strcmp(dn->name, "pci"))
+ return 0;
+
+ if (of_find_property(dn, "ibm,drc-info", NULL))
+ return rpaphp_drc_info_add_slot(dn);
+ else
+ return rpaphp_drc_add_slot(dn);
+}
EXPORT_SYMBOL_GPL(rpaphp_add_slot);
static void __exit cleanup_slots(void)
--
2.7.4
[PATCH v2 7/9] PCI: rpaphp: Annotate and correctly byte swap DRC properties
The device tree is in big endian format and any properties directly
retrieved using OF helpers that don't explicitly byte swap should
be annotated. In particular there are several places where we grab
the opaque property value for the old ibm,drc-* properties and the
ibm,my-drc-index property.
Fix this for better static checking by annotating values we know to
explicitly big endian, and byte swap where appropriate.
Signed-off-by: Tyrel Datwyler
---
drivers/pci/hotplug/rpaphp_core.c | 29 +++--
1 file changed, 15 insertions(+), 14 deletions(-)
diff --git a/drivers/pci/hotplug/rpaphp_core.c
b/drivers/pci/hotplug/rpaphp_core.c
index 75d5771..129534c 100644
--- a/drivers/pci/hotplug/rpaphp_core.c
+++ b/drivers/pci/hotplug/rpaphp_core.c
@@ -154,11 +154,11 @@ static enum pci_bus_speed get_max_bus_speed(struct slot
*slot)
return speed;
}
-static int get_children_props(struct device_node *dn, const int **drc_indexes,
- const int **drc_names, const int **drc_types,
- const int **drc_power_domains)
+static int get_children_props(struct device_node *dn, const __be32
**drc_indexes,
+ const __be32 **drc_names, const __be32
**drc_types,
+ const __be32 **drc_power_domains)
{
- const int *indexes, *names, *types, *domains;
+ const __be32 *indexes, *names, *types, *domains;
indexes = of_get_property(dn, "ibm,drc-indexes", NULL);
names = of_get_property(dn, "ibm,drc-names", NULL);
@@ -194,8 +194,8 @@ static int rpaphp_check_drc_props_v1(struct device_node
*dn, char *drc_name,
char *drc_type, unsigned int my_index)
{
char *name_tmp, *type_tmp;
- const int *indexes, *names;
- const int *types, *domains;
+ const __be32 *indexes, *names;
+ const __be32 *types, *domains;
int i, rc;
rc = get_children_props(dn->parent, &indexes, &names, &types, &domains);
@@ -208,7 +208,7 @@ static int rpaphp_check_drc_props_v1(struct device_node
*dn, char *drc_name,
/* Iterate through parent properties, looking for my-drc-index */
for (i = 0; i < be32_to_cpu(indexes[0]); i++) {
- if ((unsigned int) indexes[i + 1] == my_index)
+ if (be32_to_cpu(indexes[i + 1]) == my_index)
break;
name_tmp += (strlen(name_tmp) + 1);
@@ -267,7 +267,7 @@ static int rpaphp_check_drc_props_v2(struct device_node
*dn, char *drc_name,
int rpaphp_check_drc_props(struct device_node *dn, char *drc_name,
char *drc_type)
{
- const unsigned int *my_index;
+ const __be32 *my_index;
my_index = of_get_property(dn, "ibm,my-drc-index", NULL);
if (!my_index) {
@@ -277,10 +277,10 @@ int rpaphp_check_drc_props(struct device_node *dn, char
*drc_name,
if (of_find_property(dn->parent, "ibm,drc-info", NULL))
return rpaphp_check_drc_props_v2(dn, drc_name, drc_type,
- *my_index);
+ be32_to_cpu(*my_index));
else
return rpaphp_check_drc_props_v1(dn, drc_name, drc_type,
- *my_index);
+ be32_to_cpu(*my_index));
}
EXPORT_SYMBOL_GPL(rpaphp_check_drc_props);
@@ -311,10 +311,11 @@ static int is_php_type(char *drc_type)
* for built-in pci slots (even when the built-in slots are
* dlparable.)
*/
-static int is_php_dn(struct device_node *dn, const int **indexes,
- const int **names, const int **types, const int **power_domains)
+static int is_php_dn(struct device_node *dn, const __be32 **indexes,
+const __be32 **names, const __be32 **types,
+const __be32 **power_domains)
{
- const int *drc_types;
+ const __be32 *drc_types;
int rc;
rc = get_children_props(dn, indexes, names, &drc_types, power_domains);
@@ -374,7 +375,7 @@ static int rpaphp_drc_add_slot(struct device_node *dn)
struct slot *slot;
int retval = 0;
int i;
- const int *indexes, *names, *types, *power_domains;
+ const __be32 *indexes, *names, *types, *power_domains;
char *name, *type;
/* If this is not a hotplug slot, return without doing anything. */
--
2.7.4
[PATCH v2 0/9] Fixes and Enablement of ibm,drc-info property
There was a previous effort to add support for the PAPR architected ibm,drc-info property. This property provides a more memory compact representation of a parition's Dynamic Reconfig Connectors (DRC). These can otherwise be thought of as currently partitioned, or available but yet to be partitioned system resources such as cpus, memory, and physical/logical IOA devices. The initial implementation proved buggy and was fully turned off by disabling the bit in the appropriate CAS support vector. We now have PowerVM firmware in the field that supports this new property, and further to support partitions with 24TB+ of possible memory this property is required to perform platform migration. This series fixs the short comings of the previous submission in the areas of general implementation, cpu hotplug, and IOA hotplug. v2 changelog: Cover Letter: fixed up spelling errors (mpe, tfalcon) Patch 3: added comment regarding indexing of drc values (tfalcon) split drc-index and drc-info logic into multiple functions for collecting cpu drc's for dlpar (mpe) Patch 7: fix up a couple more sparse warnings (mpe) Tyrel Datwyler (9): powerpc/pseries: Fix bad drc_index_start value parsing of drc-info entry powerpc/pseries: Fix drc-info mappings of logical cpus to drc-index powerpc/pseries: Add cpu DLPAR support for drc-info property PCI: rpaphp: Fix up pointer to first drc-info entry PCI: rpaphp: Don't rely on firmware feature to imply drc-info support PCI: rpaphp: Add drc-info support for hotplug slot registration PCI: rpaphp: Annotate and correctly byte swap DRC properties PCI: rpaphp: Correctly match ibm,my-drc-index to drc-name when using drc-info powerpc/pseries: Enable support for ibm,drc-info property arch/powerpc/kernel/prom_init.c | 2 +- arch/powerpc/platforms/pseries/hotplug-cpu.c| 127 +--- arch/powerpc/platforms/pseries/of_helpers.c | 8 +- arch/powerpc/platforms/pseries/pseries_energy.c | 23 ++--- drivers/pci/hotplug/rpaphp_core.c | 127 +--- 5 files changed, 216 insertions(+), 71 deletions(-) -- 2.7.4
[PATCH v2 4/9] PCI: rpaphp: Fix up pointer to first drc-info entry
The first entry of the ibm,drc-info property is an int encoded count
of the number of drc-info entries that follow. The "value" pointer
returned by of_prop_next_u32() is still pointing at the this value
when we call of_read_drc_info_cell(), but the helper function
expects that value to be pointing at the first element of an entry.
Fix up by incrementing the "value" pointer to point at the first
element of the first drc-info entry prior.
Signed-off-by: Tyrel Datwyler
---
drivers/pci/hotplug/rpaphp_core.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/pci/hotplug/rpaphp_core.c
b/drivers/pci/hotplug/rpaphp_core.c
index 18627bb..e350264 100644
--- a/drivers/pci/hotplug/rpaphp_core.c
+++ b/drivers/pci/hotplug/rpaphp_core.c
@@ -239,6 +239,8 @@ static int rpaphp_check_drc_props_v2(struct device_node
*dn, char *drc_name,
value = of_prop_next_u32(info, NULL, &entries);
if (!value)
return -EINVAL;
+ else
+ value++;
for (j = 0; j < entries; j++) {
of_read_drc_info_cell(&info, &value, &drc);
--
2.7.4
Re: [PATCH 3/9] powerpc/pseries: Add cpu DLPAR support for drc-info property
On 11/5/19 8:55 AM, Thomas Falcon wrote:
>
> On 11/5/19 9:24 AM, Tyrel Datwyler wrote:
>> From: Tyrel Datwyler
>>
>> Older firmwares provided information about Dynamic Reconfig
>> Connectors (DRC) through several device tree properties, namely
>> ibm,drc-types, ibm,drc-indexes, ibm,drc-names, and
>> ibm,drc-power-domains. New firmwares have the ability to present this
>> same information in a much condensed format through a device tree
>> property called ibm,drc-info.
>>
>> The existing cpu DLPAR hotplug code only understands the older DRC
>> property format when validating the drc-index of a cpu during a
>> hotplug add. This updates those code paths to use the ibm,drc-info
>> property, when present, instead for validation.
>>
>> Signed-off-by: Tyrel Datwyler
>> ---
>> arch/powerpc/platforms/pseries/hotplug-cpu.c | 101
>> ++-
>> 1 file changed, 85 insertions(+), 16 deletions(-)
>>
>> diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c
>> b/arch/powerpc/platforms/pseries/hotplug-cpu.c
>> index bbda646..9ba006c 100644
>> --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
>> +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
>> @@ -407,17 +407,58 @@ static bool dlpar_cpu_exists(struct device_node
>> *parent,
>> u32 drc_index)
>> return found;
>> }
>>
>> +static bool drc_info_valid_index(struct device_node *parent, u32 drc_index)
>> +{
>> + struct property *info;
>> + struct of_drc_info drc;
>> + const __be32 *value;
>> + int count, i, j;
>> +
>> + info = of_find_property(parent, "ibm,drc-info", NULL);
>> + if (!info)
>> + return false;
>> +
>> + value = of_prop_next_u32(info, NULL, &count);
>> +
>> + /* First value of ibm,drc-info is number of drc-info records */
>> + if (value)
>> + value++;
>> + else
>> + return false;
>> +
>> + for (i = 0; i < count; i++) {
>> + if (of_read_drc_info_cell(&info, &value, &drc))
>> + return false;
>> +
>> + if (strncmp(drc.drc_type, "CPU", 3))
>> + break;
>> +
>> + if (drc_index > drc.last_drc_index)
>> + continue;
>> +
>> + for (j = 0; j < drc.num_sequential_elems; j++)
>> + if (drc_index == (drc.drc_index_start + (drc.sequential_inc *
>> j)))
>> + return true;
>> + }
>> +
>> + return false;
>> +}
>> +
>> static bool valid_cpu_drc_index(struct device_node *parent, u32 drc_index)
>> {
>> bool found = false;
>> int rc, index;
>>
>> - index = 0;
>> + if (of_find_property(parent, "ibm,drc-info", NULL))
>> + return drc_info_valid_index(parent, drc_index);
>> +
>> + index = 1;
>
> Hi, this change was confusing to me until I continued reading the patch and
> saw
> the comment below regarding the first element of the ibm,drc-info property.
> Would it be good to have a similar comment here too?
>
Yeah, clearly wouldn't hurt. Probably should split it out into a separate fix
prior to this patch.
>
>> while (!found) {
>> u32 drc;
>>
>> rc = of_property_read_u32_index(parent, "ibm,drc-indexes",
>> index++, &drc);
>> +
>
> Another nitpick but this could be cleaned up.
Yep, noticed the newline addition after I'd already sent it out.
-Tyrel
>
> Thanks,
>
> Tom
>
>
>> if (rc)
>> break;
>>
>> @@ -720,8 +761,11 @@ static int dlpar_cpu_remove_by_count(u32 cpus_to_remove)
>> static int find_dlpar_cpus_to_add(u32 *cpu_drcs, u32 cpus_to_add)
>> {
>> struct device_node *parent;
>> + struct property *info;
>> int cpus_found = 0;
>> int index, rc;
>> + int i, j;
>> + u32 drc_index;
>>
>> parent = of_find_node_by_path("/cpus");
>> if (!parent) {
>> @@ -730,24 +774,49 @@ static int find_dlpar_cpus_to_add(u32 *cpu_drcs, u32
>> cpus_to_add)
>> return -1;
>> }
>>
>> - /* Search the ibm,drc-indexes array for possible CPU drcs to
>> - * add. Note that the format of the ibm,drc-indexes array is
>> - * the number of entries in the array followed by the array
>> - * of drc values so we start looking at index = 1.
>> - */
>> -
Re: [PATCH 0/9] Fixes and Enablement of ibm,drc-info property
On 11/5/19 9:03 AM, Thomas Falcon wrote: > > On 11/5/19 9:24 AM, Tyrel Datwyler wrote: > > Hi, just pointing out a few typos... Damn, I thought I squashed them all the second time around. >> There was a previous effort to add support for the PAPR >> architected ibm,drc-info property. This property provides a more >> memory compact representation of a paritions Dynamic Reconfig > s/paritions/partition's >> Connectors (DRC). These can otherwise be thought of as currently >> partitioned, or available but yet to be partitioned system resources >> such as cpus, memory, and physical/logical IOA devices. >> >> The initial implementation proved buggy and was fully turned of by > > s/turned of/turned off > >> disabling the bit in the appropriate CAS support vector. We now have >> PowerVM firmware in the field that supports this new property, and >> further to suppport partitions with 24TB+ of possible memory this > s/suppport/support >> property is required to perform platform migration. >> >> This serious fixs the short comings of the previous submission > > Either "seriously fixes the shortcomings", or "fixes the serious > shortcomings?" Should be "series" as in this "patch series". -Tyrel > > Thanks, > > Tom > >> in the areas of general implementation, cpu hotplug, and IOA hotplug. >> >> Tyrel Datwyler (9): >> powerpc/pseries: Fix bad drc_index_start value parsing of drc-info >> entry >> powerpc/pseries: Fix drc-info mappings of logical cpus to drc-index >> powerpc/pseries: Add cpu DLPAR support for drc-info property >> PCI: rpaphp: Fix up pointer to first drc-info entry >> PCI: rpaphp: Don't rely on firmware feature to imply drc-info support >> PCI: rpaphp: Add drc-info support for hotplug slot registration >> PCI: rpaphp: annotate and correctly byte swap DRC properties >> PCI: rpaphp: Correctly match ibm,my-drc-index to drc-name when using >> drc-info >> powerpc/pseries: Enable support for ibm,drc-info property >> >> arch/powerpc/kernel/prom_init.c | 2 +- >> arch/powerpc/platforms/pseries/hotplug-cpu.c | 101 --- >> arch/powerpc/platforms/pseries/of_helpers.c | 8 +- >> arch/powerpc/platforms/pseries/pseries_energy.c | 23 ++--- >> drivers/pci/hotplug/rpaphp_core.c | 124 >> +--- >> 5 files changed, 187 insertions(+), 71 deletions(-) >>
[PATCH 2/9] powerpc/pseries: Fix drc-info mappings of logical cpus to drc-index
There are a couple subtle errors in the mapping between cpu-ids and a
cpus associated drc-index when using the new ibm,drc-info property.
The first is that while drc-info may have been a supported firmware
feature at boot it is possible we have migrated to a CEC with older
firmware that doesn't support the ibm,drc-info property. In that case
the device tree would have been updated after migration to remove the
ibm,drc-info property and replace it with the older style ibm,drc-*
properties for types, indexes, names, and power-domains.
The second is that the first value of the ibm,drc-info property is
the int encoded count of drc-info entries. As such "value" returned
by of_prop_next_u32() is pointing at that count, and not the first
element of the first drc-info entry as is expected by the
of_read_drc_info_cell() helper.
Fix the first by ignoring DRC-INFO firmware feature and instead
testing directly for ibm,drc-info, and then falling back to the
old style ibm,drc-indexes in the case it doesn't exit.
Fix the second by incrementing value to the next element prior to
parsing drc-info entries.
Signed-off-by: Tyrel Datwyler
---
arch/powerpc/platforms/pseries/pseries_energy.c | 23 ++-
1 file changed, 10 insertions(+), 13 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/pseries_energy.c
b/arch/powerpc/platforms/pseries/pseries_energy.c
index a96874f..09e98d3 100644
--- a/arch/powerpc/platforms/pseries/pseries_energy.c
+++ b/arch/powerpc/platforms/pseries/pseries_energy.c
@@ -36,6 +36,7 @@ static int sysfs_entries;
static u32 cpu_to_drc_index(int cpu)
{
struct device_node *dn = NULL;
+ struct property *info;
int thread_index;
int rc = 1;
u32 ret = 0;
@@ -47,20 +48,18 @@ static u32 cpu_to_drc_index(int cpu)
/* Convert logical cpu number to core number */
thread_index = cpu_core_index_of_thread(cpu);
- if (firmware_has_feature(FW_FEATURE_DRC_INFO)) {
- struct property *info = NULL;
+ info = of_find_property(dn, "ibm,drc-info", NULL);
+ if (info) {
struct of_drc_info drc;
int j;
u32 num_set_entries;
const __be32 *value;
- info = of_find_property(dn, "ibm,drc-info", NULL);
- if (info == NULL)
- goto err_of_node_put;
-
value = of_prop_next_u32(info, NULL, &num_set_entries);
if (!value)
goto err_of_node_put;
+ else
+ value++;
for (j = 0; j < num_set_entries; j++) {
@@ -110,6 +109,7 @@ static u32 cpu_to_drc_index(int cpu)
static int drc_index_to_cpu(u32 drc_index)
{
struct device_node *dn = NULL;
+ struct property *info;
const int *indexes;
int thread_index = 0, cpu = 0;
int rc = 1;
@@ -117,21 +117,18 @@ static int drc_index_to_cpu(u32 drc_index)
dn = of_find_node_by_path("/cpus");
if (dn == NULL)
goto err;
-
- if (firmware_has_feature(FW_FEATURE_DRC_INFO)) {
- struct property *info = NULL;
+ info = of_find_property(dn, "ibm,drc-info", NULL);
+ if (info) {
struct of_drc_info drc;
int j;
u32 num_set_entries;
const __be32 *value;
- info = of_find_property(dn, "ibm,drc-info", NULL);
- if (info == NULL)
- goto err_of_node_put;
-
value = of_prop_next_u32(info, NULL, &num_set_entries);
if (!value)
goto err_of_node_put;
+ else
+ value++;
for (j = 0; j < num_set_entries; j++) {
--
2.7.4
[PATCH 8/9] PCI: rpaphp: Correctly match ibm, my-drc-index to drc-name when using drc-info
The newer ibm,drc-info property is a condensed description of the old
ibm,drc-* properties (ie. names, types, indexes, and power-domains).
When matching a drc-index to a drc-name we need to verify that the
index is within the start and last drc-index range and map it to a
drc-name using the drc-name-prefix and logical index.
Fix the mapping by checking that the index is within the range of the
current drc-info entry, and build the name from the drc-name-prefix
and by adding the starting drc-name-suffix value with the sequential
index of subtracting ibm,my-drc-index from this entries
drc-start-index.
Signed-off-by: Tyrel Datwyler
---
drivers/pci/hotplug/rpaphp_core.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/hotplug/rpaphp_core.c
b/drivers/pci/hotplug/rpaphp_core.c
index eabc0c51..5327606 100644
--- a/drivers/pci/hotplug/rpaphp_core.c
+++ b/drivers/pci/hotplug/rpaphp_core.c
@@ -248,9 +248,10 @@ static int rpaphp_check_drc_props_v2(struct device_node
*dn, char *drc_name,
/* Should now know end of current entry */
/* Found it */
- if (my_index <= drc.last_drc_index) {
+ if (my_index >= drc.drc_index_start && my_index <=
drc.last_drc_index) {
+ int index = my_index - drc.drc_index_start;
sprintf(cell_drc_name, "%s%d", drc.drc_name_prefix,
- my_index);
+ drc.drc_name_suffix_start + index);
break;
}
}
--
2.7.4
[PATCH 9/9] powerpc/pseries: Enable support for ibm, drc-info property
Advertise client support for the PAPR architected ibm,drc-info device
tree property during CAS handshake.
Signed-off-by: Tyrel Datwyler
---
arch/powerpc/kernel/prom_init.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
index a4e7762..2ca9966 100644
--- a/arch/powerpc/kernel/prom_init.c
+++ b/arch/powerpc/kernel/prom_init.c
@@ -1053,7 +1053,7 @@ static const struct ibm_arch_vec
ibm_architecture_vec_template __initconst = {
.reserved2 = 0,
.reserved3 = 0,
.subprocessors = 1,
- .byte22 = OV5_FEAT(OV5_DRMEM_V2),
+ .byte22 = OV5_FEAT(OV5_DRMEM_V2) | OV5_FEAT(OV5_DRC_INFO),
.intarch = 0,
.mmu = 0,
.hash_ext = 0,
--
2.7.4
[PATCH 7/9] PCI: rpaphp: annotate and correctly byte swap DRC properties
The device tree is in big endian format and any properties directly
retrieved using OF helpers that don't explicitly byte swap should
be annotated. In particular there are several places where we grab
the opaque property value for the old ibm,drc-* properties and the
ibm,my-drc-index property.
Fix this for better static checking by annotating values we know to
explicitly big endian, and byte swap where appropriate.
Signed-off-by: Tyrel Datwyler
---
drivers/pci/hotplug/rpaphp_core.c | 26 +-
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/drivers/pci/hotplug/rpaphp_core.c
b/drivers/pci/hotplug/rpaphp_core.c
index 75d5771..eabc0c51 100644
--- a/drivers/pci/hotplug/rpaphp_core.c
+++ b/drivers/pci/hotplug/rpaphp_core.c
@@ -154,9 +154,9 @@ static enum pci_bus_speed get_max_bus_speed(struct slot
*slot)
return speed;
}
-static int get_children_props(struct device_node *dn, const int **drc_indexes,
- const int **drc_names, const int **drc_types,
- const int **drc_power_domains)
+static int get_children_props(struct device_node *dn, const __be32
**drc_indexes,
+ const __be32 **drc_names, const __be32 **drc_types,
+ const __be32 **drc_power_domains)
{
const int *indexes, *names, *types, *domains;
@@ -194,8 +194,8 @@ static int rpaphp_check_drc_props_v1(struct device_node
*dn, char *drc_name,
char *drc_type, unsigned int my_index)
{
char *name_tmp, *type_tmp;
- const int *indexes, *names;
- const int *types, *domains;
+ const __be32 *indexes, *names;
+ const __be32 *types, *domains;
int i, rc;
rc = get_children_props(dn->parent, &indexes, &names, &types, &domains);
@@ -208,7 +208,7 @@ static int rpaphp_check_drc_props_v1(struct device_node
*dn, char *drc_name,
/* Iterate through parent properties, looking for my-drc-index */
for (i = 0; i < be32_to_cpu(indexes[0]); i++) {
- if ((unsigned int) indexes[i + 1] == my_index)
+ if (be32_to_cpu(indexes[i + 1]) == my_index)
break;
name_tmp += (strlen(name_tmp) + 1);
@@ -267,7 +267,7 @@ static int rpaphp_check_drc_props_v2(struct device_node
*dn, char *drc_name,
int rpaphp_check_drc_props(struct device_node *dn, char *drc_name,
char *drc_type)
{
- const unsigned int *my_index;
+ const __be32 *my_index;
my_index = of_get_property(dn, "ibm,my-drc-index", NULL);
if (!my_index) {
@@ -277,10 +277,10 @@ int rpaphp_check_drc_props(struct device_node *dn, char
*drc_name,
if (of_find_property(dn->parent, "ibm,drc-info", NULL))
return rpaphp_check_drc_props_v2(dn, drc_name, drc_type,
- *my_index);
+ be32_to_cpu(*my_index));
else
return rpaphp_check_drc_props_v1(dn, drc_name, drc_type,
- *my_index);
+ be32_to_cpu(*my_index));
}
EXPORT_SYMBOL_GPL(rpaphp_check_drc_props);
@@ -311,10 +311,10 @@ static int is_php_type(char *drc_type)
* for built-in pci slots (even when the built-in slots are
* dlparable.)
*/
-static int is_php_dn(struct device_node *dn, const int **indexes,
- const int **names, const int **types, const int **power_domains)
+static int is_php_dn(struct device_node *dn, const __be32 **indexes,
+ const __be32 **names, const __be32 **types, const __be32
**power_domains)
{
- const int *drc_types;
+ const __be32 *drc_types;
int rc;
rc = get_children_props(dn, indexes, names, &drc_types, power_domains);
@@ -374,7 +374,7 @@ static int rpaphp_drc_add_slot(struct device_node *dn)
struct slot *slot;
int retval = 0;
int i;
- const int *indexes, *names, *types, *power_domains;
+ const __be32 *indexes, *names, *types, *power_domains;
char *name, *type;
/* If this is not a hotplug slot, return without doing anything. */
--
2.7.4
[PATCH 4/9] PCI: rpaphp: Fix up pointer to first drc-info entry
The first entry of the ibm,drc-info property is an int encoded count
of the number of drc-info entries that follow. The "value" pointer
returned by of_prop_next_u32() is still pointing at the this value
when we call of_read_drc_info_cell(), but the helper function
expects that value to be pointing at the first element of an entry.
Fix up by incrementing the "value" pointer to point at the first
element of the first drc-info entry prior.
Signed-off-by: Tyrel Datwyler
---
drivers/pci/hotplug/rpaphp_core.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/pci/hotplug/rpaphp_core.c
b/drivers/pci/hotplug/rpaphp_core.c
index 18627bb..e350264 100644
--- a/drivers/pci/hotplug/rpaphp_core.c
+++ b/drivers/pci/hotplug/rpaphp_core.c
@@ -239,6 +239,8 @@ static int rpaphp_check_drc_props_v2(struct device_node
*dn, char *drc_name,
value = of_prop_next_u32(info, NULL, &entries);
if (!value)
return -EINVAL;
+ else
+ value++;
for (j = 0; j < entries; j++) {
of_read_drc_info_cell(&info, &value, &drc);
--
2.7.4
[PATCH 6/9] PCI: rpaphp: Add drc-info support for hotplug slot registration
Split physical PCI slot registration scanning into seperate routines
that support the old ibm,drc-* properties and one that supports the
new compressed ibm,drc-info property.
Signed-off-by: Tyrel Datwyler
---
drivers/pci/hotplug/rpaphp_core.c | 89 ++-
1 file changed, 69 insertions(+), 20 deletions(-)
diff --git a/drivers/pci/hotplug/rpaphp_core.c
b/drivers/pci/hotplug/rpaphp_core.c
index e18e9a0..75d5771 100644
--- a/drivers/pci/hotplug/rpaphp_core.c
+++ b/drivers/pci/hotplug/rpaphp_core.c
@@ -328,23 +328,48 @@ static int is_php_dn(struct device_node *dn, const int
**indexes,
return 1;
}
-/**
- * rpaphp_add_slot -- declare a hotplug slot to the hotplug subsystem.
- * @dn: device node of slot
- *
- * This subroutine will register a hotpluggable slot with the
- * PCI hotplug infrastructure. This routine is typically called
- * during boot time, if the hotplug slots are present at boot time,
- * or is called later, by the dlpar add code, if the slot is
- * being dynamically added during runtime.
- *
- * If the device node points at an embedded (built-in) slot, this
- * routine will just return without doing anything, since embedded
- * slots cannot be hotplugged.
- *
- * To remove a slot, it suffices to call rpaphp_deregister_slot().
- */
-int rpaphp_add_slot(struct device_node *dn)
+static int rpaphp_drc_info_add_slot(struct device_node *dn)
+{
+ struct slot *slot;
+ struct property *info;
+ struct of_drc_info drc;
+ char drc_name[MAX_DRC_NAME_LEN];
+ const __be32 *cur;
+ u32 count;
+ int retval = 0;
+
+ info = of_find_property(dn, "ibm,drc-info", NULL);
+ if (!info)
+ return 0;
+
+ cur = of_prop_next_u32(info, NULL, &count);
+ if (cur)
+ cur++;
+ else
+ return 0;
+
+ of_read_drc_info_cell(&info, &cur, &drc);
+ if (!is_php_type(drc.drc_type))
+ return 0;
+
+ sprintf(drc_name, "%s%d", drc.drc_name_prefix,
drc.drc_name_suffix_start);
+
+ slot = alloc_slot_struct(dn, drc.drc_index_start, drc_name,
drc.drc_power_domain);
+ if (!slot)
+ return -ENOMEM;
+
+ slot->type = simple_strtoul(drc.drc_type, NULL, 10);
+ retval = rpaphp_enable_slot(slot);
+ if (!retval)
+ retval = rpaphp_register_slot(slot);
+
+ if (retval)
+ dealloc_slot_struct(slot);
+
+ return retval;
+}
+
+static int rpaphp_drc_add_slot(struct device_node *dn)
{
struct slot *slot;
int retval = 0;
@@ -352,9 +377,6 @@ int rpaphp_add_slot(struct device_node *dn)
const int *indexes, *names, *types, *power_domains;
char *name, *type;
- if (!dn->name || strcmp(dn->name, "pci"))
- return 0;
-
/* If this is not a hotplug slot, return without doing anything. */
if (!is_php_dn(dn, &indexes, &names, &types, &power_domains))
return 0;
@@ -393,6 +415,33 @@ int rpaphp_add_slot(struct device_node *dn)
/* XXX FIXME: reports a failure only if last entry in loop failed */
return retval;
}
+
+/**
+ * rpaphp_add_slot -- declare a hotplug slot to the hotplug subsystem.
+ * @dn: device node of slot
+ *
+ * This subroutine will register a hotpluggable slot with the
+ * PCI hotplug infrastructure. This routine is typically called
+ * during boot time, if the hotplug slots are present at boot time,
+ * or is called later, by the dlpar add code, if the slot is
+ * being dynamically added during runtime.
+ *
+ * If the device node points at an embedded (built-in) slot, this
+ * routine will just return without doing anything, since embedded
+ * slots cannot be hotplugged.
+ *
+ * To remove a slot, it suffices to call rpaphp_deregister_slot().
+ */
+int rpaphp_add_slot(struct device_node *dn)
+{
+ if (!dn->name || strcmp(dn->name, "pci"))
+ return 0;
+
+ if (of_find_property(dn, "ibm,drc-info", NULL))
+ return rpaphp_drc_info_add_slot(dn);
+ else
+ return rpaphp_drc_add_slot(dn);
+}
EXPORT_SYMBOL_GPL(rpaphp_add_slot);
static void __exit cleanup_slots(void)
--
2.7.4
[PATCH 1/9] powerpc/pseries: Fix bad drc_index_start value parsing of drc-info entry
The ibm,drc-info property is an array property that contains drc-info
entries such that each entry is made up of 2 string encoded elements
followed by 5 int encoded elements. The of_read_drc_info_cell()
helper contains comments that correctly name the expected elements
and their encoding. However, the usage of of_prop_next_string() and
of_prop_next_u32() introduced a subtle skippage of the first u32.
This is a result of of_prop_next_string() returning a pointer to the
next property value which is not a string, but actually a (__be32 *).
As, a result the following call to of_prop_next_u32() passes over the
current int encoded value and actually stores the next one wrongly.
Simply endian swap the current value in place after reading the first
two string values. The remaining int encoded values can then be read
correctly using of_prop_next_u32().
Signed-off-by: Tyrel Datwyler
---
arch/powerpc/platforms/pseries/of_helpers.c | 8 +++-
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/of_helpers.c
b/arch/powerpc/platforms/pseries/of_helpers.c
index 6df192f..66dfd82 100644
--- a/arch/powerpc/platforms/pseries/of_helpers.c
+++ b/arch/powerpc/platforms/pseries/of_helpers.c
@@ -45,14 +45,14 @@ struct device_node *pseries_of_derive_parent(const char
*path)
int of_read_drc_info_cell(struct property **prop, const __be32 **curval,
struct of_drc_info *data)
{
- const char *p;
+ const char *p = (char *)(*curval);
const __be32 *p2;
if (!data)
return -EINVAL;
/* Get drc-type:encode-string */
- p = data->drc_type = (char*) (*curval);
+ data->drc_type = (char *)p;
p = of_prop_next_string(*prop, p);
if (!p)
return -EINVAL;
@@ -65,9 +65,7 @@ int of_read_drc_info_cell(struct property **prop, const
__be32 **curval,
/* Get drc-index-start:encode-int */
p2 = (const __be32 *)p;
- p2 = of_prop_next_u32(*prop, p2, &data->drc_index_start);
- if (!p2)
- return -EINVAL;
+ data->drc_index_start = be32_to_cpu(*p2);
/* Get drc-name-suffix-start:encode-int */
p2 = of_prop_next_u32(*prop, p2, &data->drc_name_suffix_start);
--
2.7.4
[PATCH 3/9] powerpc/pseries: Add cpu DLPAR support for drc-info property
From: Tyrel Datwyler
Older firmwares provided information about Dynamic Reconfig
Connectors (DRC) through several device tree properties, namely
ibm,drc-types, ibm,drc-indexes, ibm,drc-names, and
ibm,drc-power-domains. New firmwares have the ability to present this
same information in a much condensed format through a device tree
property called ibm,drc-info.
The existing cpu DLPAR hotplug code only understands the older DRC
property format when validating the drc-index of a cpu during a
hotplug add. This updates those code paths to use the ibm,drc-info
property, when present, instead for validation.
Signed-off-by: Tyrel Datwyler
---
arch/powerpc/platforms/pseries/hotplug-cpu.c | 101 ++-
1 file changed, 85 insertions(+), 16 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c
b/arch/powerpc/platforms/pseries/hotplug-cpu.c
index bbda646..9ba006c 100644
--- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
+++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
@@ -407,17 +407,58 @@ static bool dlpar_cpu_exists(struct device_node *parent,
u32 drc_index)
return found;
}
+static bool drc_info_valid_index(struct device_node *parent, u32 drc_index)
+{
+ struct property *info;
+ struct of_drc_info drc;
+ const __be32 *value;
+ int count, i, j;
+
+ info = of_find_property(parent, "ibm,drc-info", NULL);
+ if (!info)
+ return false;
+
+ value = of_prop_next_u32(info, NULL, &count);
+
+ /* First value of ibm,drc-info is number of drc-info records */
+ if (value)
+ value++;
+ else
+ return false;
+
+ for (i = 0; i < count; i++) {
+ if (of_read_drc_info_cell(&info, &value, &drc))
+ return false;
+
+ if (strncmp(drc.drc_type, "CPU", 3))
+ break;
+
+ if (drc_index > drc.last_drc_index)
+ continue;
+
+ for (j = 0; j < drc.num_sequential_elems; j++)
+ if (drc_index == (drc.drc_index_start +
(drc.sequential_inc * j)))
+ return true;
+ }
+
+ return false;
+}
+
static bool valid_cpu_drc_index(struct device_node *parent, u32 drc_index)
{
bool found = false;
int rc, index;
- index = 0;
+ if (of_find_property(parent, "ibm,drc-info", NULL))
+ return drc_info_valid_index(parent, drc_index);
+
+ index = 1;
while (!found) {
u32 drc;
rc = of_property_read_u32_index(parent, "ibm,drc-indexes",
index++, &drc);
+
if (rc)
break;
@@ -720,8 +761,11 @@ static int dlpar_cpu_remove_by_count(u32 cpus_to_remove)
static int find_dlpar_cpus_to_add(u32 *cpu_drcs, u32 cpus_to_add)
{
struct device_node *parent;
+ struct property *info;
int cpus_found = 0;
int index, rc;
+ int i, j;
+ u32 drc_index;
parent = of_find_node_by_path("/cpus");
if (!parent) {
@@ -730,24 +774,49 @@ static int find_dlpar_cpus_to_add(u32 *cpu_drcs, u32
cpus_to_add)
return -1;
}
- /* Search the ibm,drc-indexes array for possible CPU drcs to
-* add. Note that the format of the ibm,drc-indexes array is
-* the number of entries in the array followed by the array
-* of drc values so we start looking at index = 1.
-*/
- index = 1;
- while (cpus_found < cpus_to_add) {
- u32 drc;
+ info = of_find_property(parent, "ibm,drc-info", NULL);
+ if (info) {
+ struct of_drc_info drc;
+ const __be32 *value;
+ int count;
- rc = of_property_read_u32_index(parent, "ibm,drc-indexes",
- index++, &drc);
- if (rc)
- break;
+ value = of_prop_next_u32(info, NULL, &count);
+ if (value)
+ value++;
- if (dlpar_cpu_exists(parent, drc))
- continue;
+ for (i = 0; i < count; i++) {
+ of_read_drc_info_cell(&info, &value, &drc);
+ if (strncmp(drc.drc_type, "CPU", 3))
+ break;
+
+ for (j = 0; j < drc.num_sequential_elems && cpus_found
< cpus_to_add; j++) {
+ drc_index = drc.drc_index_start +
(drc.sequential_inc * j);
+
+ if (dlpar_cpu_exists(parent, drc_index))
+ continue;
+
+ cpu_drcs[cpus_found++] =
[PATCH 5/9] PCI: rpaphp: Don't rely on firmware feature to imply drc-info support
In the event that the partition is migrated to a platform with older firmware that doesn't support the ibm,drc-info property the device tree is modified to remove the ibm,drc-info property and replace it with the older style ibm,drc-* properties for types, names, indexes, and power-domains. One of the requirements of the drc-info firmware feature is the the client is able to handle both the new property, and old properties. Therefore we can't rely on the firmware feature alone to dictate which property is currently present in the device tree. Fix this short coming by checking explicitly for the ibm,drc-info property, and falling back to the older ibm,drc-* properties if it doesn't exist. Signed-off-by: Tyrel Datwyler --- drivers/pci/hotplug/rpaphp_core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/pci/hotplug/rpaphp_core.c b/drivers/pci/hotplug/rpaphp_core.c index e350264..e18e9a0 100644 --- a/drivers/pci/hotplug/rpaphp_core.c +++ b/drivers/pci/hotplug/rpaphp_core.c @@ -275,7 +275,7 @@ int rpaphp_check_drc_props(struct device_node *dn, char *drc_name, return -EINVAL; } - if (firmware_has_feature(FW_FEATURE_DRC_INFO)) + if (of_find_property(dn->parent, "ibm,drc-info", NULL)) return rpaphp_check_drc_props_v2(dn, drc_name, drc_type, *my_index); else -- 2.7.4
[PATCH 0/9] Fixes and Enablement of ibm,drc-info property
There was a previous effort to add support for the PAPR architected ibm,drc-info property. This property provides a more memory compact representation of a paritions Dynamic Reconfig Connectors (DRC). These can otherwise be thought of as currently partitioned, or available but yet to be partitioned system resources such as cpus, memory, and physical/logical IOA devices. The initial implementation proved buggy and was fully turned of by disabling the bit in the appropriate CAS support vector. We now have PowerVM firmware in the field that supports this new property, and further to suppport partitions with 24TB+ of possible memory this property is required to perform platform migration. This serious fixs the short comings of the previous submission in the areas of general implementation, cpu hotplug, and IOA hotplug. Tyrel Datwyler (9): powerpc/pseries: Fix bad drc_index_start value parsing of drc-info entry powerpc/pseries: Fix drc-info mappings of logical cpus to drc-index powerpc/pseries: Add cpu DLPAR support for drc-info property PCI: rpaphp: Fix up pointer to first drc-info entry PCI: rpaphp: Don't rely on firmware feature to imply drc-info support PCI: rpaphp: Add drc-info support for hotplug slot registration PCI: rpaphp: annotate and correctly byte swap DRC properties PCI: rpaphp: Correctly match ibm,my-drc-index to drc-name when using drc-info powerpc/pseries: Enable support for ibm,drc-info property arch/powerpc/kernel/prom_init.c | 2 +- arch/powerpc/platforms/pseries/hotplug-cpu.c| 101 --- arch/powerpc/platforms/pseries/of_helpers.c | 8 +- arch/powerpc/platforms/pseries/pseries_energy.c | 23 ++--- drivers/pci/hotplug/rpaphp_core.c | 124 +--- 5 files changed, 187 insertions(+), 71 deletions(-) -- 2.7.4
Re: [RFC PATCH 0/9] Fixes and Enablement of ibm,drc-info property
On 10/1/19 1:02 PM, Bjorn Helgaas wrote: > On Tue, Oct 01, 2019 at 01:12:05AM -0500, Tyrel Datwyler wrote: >> There was an initial previous effort yo add support for the PAPR >> architected ibm,drc-info property. This property provides a more >> memory compact representation of a paritions Dynamic Reconfig >> Connectors (DRC). These can otherwise be thought of the currently >> partitioned, or available, but yet to be partitioned, system resources >> such as cpus, memory, and physical/logical IOA devices. >> >> The initial implementation proved buggy and was fully turned of by >> disabling the bit in the appropriate CAS support vector. We now have >> PowerVM firmware in the field that supports this new property, and >> further to suppport partitions with 24TB+ or possible memory this >> property is required to perform platform migration. >> >> This serious fixup the short comings of the previous implementation >> in the areas of general implementation, cpu hotplug, and IOA hotplug. >> >> Tyrel Datwyler (9): >> powerpc/pseries: add cpu DLPAR support for drc-info property >> powerpc/pseries: fix bad drc_index_start value parsing of drc-info >> entry >> powerpc/pseries: fix drc-info mappings of logical cpus to drc-index >> PCI: rpaphp: fix up pointer to first drc-info entry >> PCI: rpaphp: don't rely on firmware feature to imply drc-info support >> PCI: rpaphp: add drc-info support for hotplug slot registration >> PCI: rpaphp: annotate and correctly byte swap DRC properties >> PCI: rpaphp: correctly match ibm,my-drc-index to drc-name when using >> drc-info >> powerpc: Enable support for ibm,drc-info property >> >> arch/powerpc/kernel/prom_init.c | 2 +- >> arch/powerpc/platforms/pseries/hotplug-cpu.c| 117 -- >> arch/powerpc/platforms/pseries/of_helpers.c | 8 +- >> arch/powerpc/platforms/pseries/pseries_energy.c | 23 ++--- >> drivers/pci/hotplug/rpaphp_core.c | 124 >> +--- >> 5 files changed, 191 insertions(+), 83 deletions(-) > > Michael, I assume you'll take care of this. If I were applying, I > would capitalize the commit subjects and fix the typos in the commit > logs, e.g., > > s/the this/the/ > s/the the/that the/ > s/short coming/shortcoming/ > s/seperate/separate/ > s/bid endian/big endian/ > s/were appropriate/where appropriate/ > s/name form/name from/ > > etc. git am also complains about space before tab whitespace errors. > And it adds a few lines >80 chars. > I'll fix all those up in the next spin. -Tyrel
Re: [RFC PATCH 2/9] powerpc/pseries: fix bad drc_index_start value parsing of drc-info entry
On 10/10/19 12:04 PM, Nathan Lynch wrote: > Tyrel Datwyler writes: >> The ibm,drc-info property is an array property that contains drc-info >> entries such that each entry is made up of 2 string encoded elements >> followed by 5 int encoded elements. The of_read_drc_info_cell() >> helper contains comments that correctly name the expected elements >> and their encoding. However, the usage of of_prop_next_string() and >> of_prop_next_u32() introduced a subtle skippage of the first u32. >> This is a result of of_prop_next_string() returns a pointer to the >> next property value which is not a string, but actually a (__be32 *). >> As, a result the following call to of_prop_next_u32() passes over the >> current int encoded value and actually stores the next one wrongly. >> >> Simply endian swap the current value in place after reading the first >> two string values. The remaining int encoded values can then be read >> correctly using of_prop_next_u32(). > > Good but I think it would make more sense for a fix for > of_read_drc_info_cell() to precede any patch in the series which > introduces new callers, such as patch #1. > Not sure it matters that much since everything in the series is required to properly enable a working drc-info implementation and the call already exists so it doesn't break bisectability. It ended up second in the series since testing patch 1 exposed the flaw. I'll go ahead and move it first, and add the appropriate fixes tag as well which is currently missing. -Tyrel
Re: [RFC PATCH 1/9] powerpc/pseries: add cpu DLPAR support for drc-info property
On 10/10/19 11:56 AM, Nathan Lynch wrote:
> Hi Tyrel,
>
> Tyrel Datwyler writes:
>> +static bool valid_cpu_drc_index(struct device_node *parent, u32 drc_index)
>> +{
>> +const __be32 *indexes;
>> +int i;
>> +
>> +if (of_find_property(parent, "ibm,drc-info", NULL))
>> +return drc_info_valid_index(parent, drc_index);
>> +
>> +indexes = of_get_property(parent, "ibm,drc-indexes", NULL);
>> +if (!indexes)
>> +return false;
>> +
>> +for (i = 0; i < indexes[0]; i++) {
>
> should this be:
>
> for (i = 0; i < be32_to_cpu(indexes[0]); i++) {
> ?
Yes!
>
>
>> +if (be32_to_cpu(indexes[i + 1]) == drc_index)
>> +return true;
>> +}
>> +
>> +return false;
>> }
>
> It looks like this rewrites valid_cpu_drc_index()'s existing code for
> parsing ibm,drc-indexes but I don't see the need for this.
>
> This patch would be easier to review if that were dropped or split out.
Yeah, I'll split it out. There are multiple places where we iterate over the
drc_indexes, and it is implemented several different ways. I basically picked an
implementation to use across the board. I think a better way would be just to
implement a for_each_drc_index(dn, drc_index) macro to abstract away iterator
implementation.
>
>>
>> static ssize_t dlpar_cpu_add(u32 drc_index)
>> @@ -720,8 +756,11 @@ static int dlpar_cpu_remove_by_count(u32 cpus_to_remove)
>> static int find_dlpar_cpus_to_add(u32 *cpu_drcs, u32 cpus_to_add)
>> {
>> struct device_node *parent;
>> +struct property *info;
>> +const __be32 *indexes;
>> int cpus_found = 0;
>> -int index, rc;
>> +int i, j;
>> +u32 drc_index;
>>
>> parent = of_find_node_by_path("/cpus");
>> if (!parent) {
>> @@ -730,24 +769,46 @@ static int find_dlpar_cpus_to_add(u32 *cpu_drcs, u32
>> cpus_to_add)
>> return -1;
>> }
>>
>> -/* Search the ibm,drc-indexes array for possible CPU drcs to
>> - * add. Note that the format of the ibm,drc-indexes array is
>> - * the number of entries in the array followed by the array
>> - * of drc values so we start looking at index = 1.
>> - */
>> -index = 1;
>> -while (cpus_found < cpus_to_add) {
>> -u32 drc;
>> +info = of_find_property(parent, "ibm,drc-info", NULL);
>> +if (info) {
>> +struct of_drc_info drc;
>> +const __be32 *value;
>> +int count;
>>
>> -rc = of_property_read_u32_index(parent, "ibm,drc-indexes",
>> -index++, &drc);
>> -if (rc)
>> -break;
>> +value = of_prop_next_u32(info, NULL, &count);
>> +if (value)
>> +value++;
>>
>> -if (dlpar_cpu_exists(parent, drc))
>> -continue;
>> +for (i = 0; i < count; i++) {
>> +of_read_drc_info_cell(&info, &value, &drc);
>> +if (strncmp(drc.drc_type, "CPU", 3))
>> +break;
>> +
>> +for (j = 0; j < drc.num_sequential_elems; j++) {
>> +drc_index = drc.drc_index_start +
>> (drc.sequential_inc * j);
>> +
>> +if (dlpar_cpu_exists(parent, drc_index))
>> +continue;
>>
>> -cpu_drcs[cpus_found++] = drc;
>> +cpu_drcs[cpus_found++] = drc_index;
>
> I am failing to see how this loop is limited by the cpus_to_add
> parameter as it was before this change. It looks like this will overflow
> the cpu_drcs array when cpus_to_add is less than the number of cpus
> found.
You are right. The code is picking every non-present drc_index which will
overflow the supplied buffer as you stated when there are more available indexes
than requested cpus. Will fix to bound the search.
>
> As an aside I don't understand how the add_by_count()/dlpar_cpu_exists()
> algorithm could be correct as it currently stands. It seems to pick the
> first X indexes for which a corresponding cpu node is absent, but that
> set of indexes does not necessarily match the set that is available to
> configure. Something to address separately I suppose.
I&#
[RFC PATCH 8/9] PCI: rpaphp: correctly match ibm, my-drc-index to drc-name when using drc-info
The newer ibm,drc-info property is a condensed description of the old
ibm,drc-* properties (ie. names, types, indexes, and power-domains).
When matching a drc-index to a drc-name we need to verify that the
index is within the start and last drc-index range and map it to a
drc-name using the drc-name-prefix and logical index.
Fix the mapping by checking that the index is within the range of the
current drc-info entry, and build the name form the drc-name-prefix
and by adding the starting drc-name-suffix value with the sequential
index of subtracting ibm,my-drc-index from this entries
drc-start-index.
Signed-off-by: Tyrel Datwyler
---
drivers/pci/hotplug/rpaphp_core.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/hotplug/rpaphp_core.c
b/drivers/pci/hotplug/rpaphp_core.c
index eabc0c51..5327606 100644
--- a/drivers/pci/hotplug/rpaphp_core.c
+++ b/drivers/pci/hotplug/rpaphp_core.c
@@ -248,9 +248,10 @@ static int rpaphp_check_drc_props_v2(struct device_node
*dn, char *drc_name,
/* Should now know end of current entry */
/* Found it */
- if (my_index <= drc.last_drc_index) {
+ if (my_index >= drc.drc_index_start && my_index <=
drc.last_drc_index) {
+ int index = my_index - drc.drc_index_start;
sprintf(cell_drc_name, "%s%d", drc.drc_name_prefix,
- my_index);
+ drc.drc_name_suffix_start + index);
break;
}
}
--
2.7.4
[RFC PATCH 9/9] powerpc: Enable support for ibm,drc-info property
Advertise client support for the PAPR architected ibm,drc-info device
tree property during CAS handshake.
Signed-off-by: Tyrel Datwyler
---
arch/powerpc/kernel/prom_init.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
index a4e7762..2ca9966 100644
--- a/arch/powerpc/kernel/prom_init.c
+++ b/arch/powerpc/kernel/prom_init.c
@@ -1053,7 +1053,7 @@ static const struct ibm_arch_vec
ibm_architecture_vec_template __initconst = {
.reserved2 = 0,
.reserved3 = 0,
.subprocessors = 1,
- .byte22 = OV5_FEAT(OV5_DRMEM_V2),
+ .byte22 = OV5_FEAT(OV5_DRMEM_V2) | OV5_FEAT(OV5_DRC_INFO),
.intarch = 0,
.mmu = 0,
.hash_ext = 0,
--
2.7.4
[RFC PATCH 0/9] Fixes and Enablement of ibm,drc-info property
There was an initial previous effort yo add support for the PAPR architected ibm,drc-info property. This property provides a more memory compact representation of a paritions Dynamic Reconfig Connectors (DRC). These can otherwise be thought of the currently partitioned, or available, but yet to be partitioned, system resources such as cpus, memory, and physical/logical IOA devices. The initial implementation proved buggy and was fully turned of by disabling the bit in the appropriate CAS support vector. We now have PowerVM firmware in the field that supports this new property, and further to suppport partitions with 24TB+ or possible memory this property is required to perform platform migration. This serious fixup the short comings of the previous implementation in the areas of general implementation, cpu hotplug, and IOA hotplug. Tyrel Datwyler (9): powerpc/pseries: add cpu DLPAR support for drc-info property powerpc/pseries: fix bad drc_index_start value parsing of drc-info entry powerpc/pseries: fix drc-info mappings of logical cpus to drc-index PCI: rpaphp: fix up pointer to first drc-info entry PCI: rpaphp: don't rely on firmware feature to imply drc-info support PCI: rpaphp: add drc-info support for hotplug slot registration PCI: rpaphp: annotate and correctly byte swap DRC properties PCI: rpaphp: correctly match ibm,my-drc-index to drc-name when using drc-info powerpc: Enable support for ibm,drc-info property arch/powerpc/kernel/prom_init.c | 2 +- arch/powerpc/platforms/pseries/hotplug-cpu.c| 117 -- arch/powerpc/platforms/pseries/of_helpers.c | 8 +- arch/powerpc/platforms/pseries/pseries_energy.c | 23 ++--- drivers/pci/hotplug/rpaphp_core.c | 124 +--- 5 files changed, 191 insertions(+), 83 deletions(-) -- 2.7.4
[RFC PATCH 6/9] PCI: rpaphp: add drc-info support for hotplug slot registration
Split physical PCI slot registration scanning into seperate routines
that support the old ibm,drc-* properties and one that supports the
new compressed ibm,drc-info property.
Signed-off-by: Tyrel Datwyler
---
drivers/pci/hotplug/rpaphp_core.c | 89 ++-
1 file changed, 69 insertions(+), 20 deletions(-)
diff --git a/drivers/pci/hotplug/rpaphp_core.c
b/drivers/pci/hotplug/rpaphp_core.c
index e18e9a0..75d5771 100644
--- a/drivers/pci/hotplug/rpaphp_core.c
+++ b/drivers/pci/hotplug/rpaphp_core.c
@@ -328,23 +328,48 @@ static int is_php_dn(struct device_node *dn, const int
**indexes,
return 1;
}
-/**
- * rpaphp_add_slot -- declare a hotplug slot to the hotplug subsystem.
- * @dn: device node of slot
- *
- * This subroutine will register a hotpluggable slot with the
- * PCI hotplug infrastructure. This routine is typically called
- * during boot time, if the hotplug slots are present at boot time,
- * or is called later, by the dlpar add code, if the slot is
- * being dynamically added during runtime.
- *
- * If the device node points at an embedded (built-in) slot, this
- * routine will just return without doing anything, since embedded
- * slots cannot be hotplugged.
- *
- * To remove a slot, it suffices to call rpaphp_deregister_slot().
- */
-int rpaphp_add_slot(struct device_node *dn)
+static int rpaphp_drc_info_add_slot(struct device_node *dn)
+{
+ struct slot *slot;
+ struct property *info;
+ struct of_drc_info drc;
+ char drc_name[MAX_DRC_NAME_LEN];
+ const __be32 *cur;
+ u32 count;
+ int retval = 0;
+
+ info = of_find_property(dn, "ibm,drc-info", NULL);
+ if (!info)
+ return 0;
+
+ cur = of_prop_next_u32(info, NULL, &count);
+ if (cur)
+ cur++;
+ else
+ return 0;
+
+ of_read_drc_info_cell(&info, &cur, &drc);
+ if (!is_php_type(drc.drc_type))
+ return 0;
+
+ sprintf(drc_name, "%s%d", drc.drc_name_prefix,
drc.drc_name_suffix_start);
+
+ slot = alloc_slot_struct(dn, drc.drc_index_start, drc_name,
drc.drc_power_domain);
+ if (!slot)
+ return -ENOMEM;
+
+ slot->type = simple_strtoul(drc.drc_type, NULL, 10);
+ retval = rpaphp_enable_slot(slot);
+ if (!retval)
+ retval = rpaphp_register_slot(slot);
+
+ if (retval)
+ dealloc_slot_struct(slot);
+
+ return retval;
+}
+
+static int rpaphp_drc_add_slot(struct device_node *dn)
{
struct slot *slot;
int retval = 0;
@@ -352,9 +377,6 @@ int rpaphp_add_slot(struct device_node *dn)
const int *indexes, *names, *types, *power_domains;
char *name, *type;
- if (!dn->name || strcmp(dn->name, "pci"))
- return 0;
-
/* If this is not a hotplug slot, return without doing anything. */
if (!is_php_dn(dn, &indexes, &names, &types, &power_domains))
return 0;
@@ -393,6 +415,33 @@ int rpaphp_add_slot(struct device_node *dn)
/* XXX FIXME: reports a failure only if last entry in loop failed */
return retval;
}
+
+/**
+ * rpaphp_add_slot -- declare a hotplug slot to the hotplug subsystem.
+ * @dn: device node of slot
+ *
+ * This subroutine will register a hotpluggable slot with the
+ * PCI hotplug infrastructure. This routine is typically called
+ * during boot time, if the hotplug slots are present at boot time,
+ * or is called later, by the dlpar add code, if the slot is
+ * being dynamically added during runtime.
+ *
+ * If the device node points at an embedded (built-in) slot, this
+ * routine will just return without doing anything, since embedded
+ * slots cannot be hotplugged.
+ *
+ * To remove a slot, it suffices to call rpaphp_deregister_slot().
+ */
+int rpaphp_add_slot(struct device_node *dn)
+{
+ if (!dn->name || strcmp(dn->name, "pci"))
+ return 0;
+
+ if (of_find_property(dn, "ibm,drc-info", NULL))
+ return rpaphp_drc_info_add_slot(dn);
+ else
+ return rpaphp_drc_add_slot(dn);
+}
EXPORT_SYMBOL_GPL(rpaphp_add_slot);
static void __exit cleanup_slots(void)
--
2.7.4
[RFC PATCH 7/9] PCI: rpaphp: annotate and correctly byte swap DRC properties
The device tree is in bid endian format and any properties directly
retrieved using OF helpers that don't explicitly byte swap should
be annotated. In particular there are several places where we grab
the opaque property value for the old ibm,drc-* properties and the
ibm,my-drc-index property.
Fix this for better static checking by annotating values we know to
explicitly big endian, and byte swap were appropriate.
Signed-off-by: Tyrel Datwyler
---
drivers/pci/hotplug/rpaphp_core.c | 26 +-
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/drivers/pci/hotplug/rpaphp_core.c
b/drivers/pci/hotplug/rpaphp_core.c
index 75d5771..eabc0c51 100644
--- a/drivers/pci/hotplug/rpaphp_core.c
+++ b/drivers/pci/hotplug/rpaphp_core.c
@@ -154,9 +154,9 @@ static enum pci_bus_speed get_max_bus_speed(struct slot
*slot)
return speed;
}
-static int get_children_props(struct device_node *dn, const int **drc_indexes,
- const int **drc_names, const int **drc_types,
- const int **drc_power_domains)
+static int get_children_props(struct device_node *dn, const __be32
**drc_indexes,
+ const __be32 **drc_names, const __be32 **drc_types,
+ const __be32 **drc_power_domains)
{
const int *indexes, *names, *types, *domains;
@@ -194,8 +194,8 @@ static int rpaphp_check_drc_props_v1(struct device_node
*dn, char *drc_name,
char *drc_type, unsigned int my_index)
{
char *name_tmp, *type_tmp;
- const int *indexes, *names;
- const int *types, *domains;
+ const __be32 *indexes, *names;
+ const __be32 *types, *domains;
int i, rc;
rc = get_children_props(dn->parent, &indexes, &names, &types, &domains);
@@ -208,7 +208,7 @@ static int rpaphp_check_drc_props_v1(struct device_node
*dn, char *drc_name,
/* Iterate through parent properties, looking for my-drc-index */
for (i = 0; i < be32_to_cpu(indexes[0]); i++) {
- if ((unsigned int) indexes[i + 1] == my_index)
+ if (be32_to_cpu(indexes[i + 1]) == my_index)
break;
name_tmp += (strlen(name_tmp) + 1);
@@ -267,7 +267,7 @@ static int rpaphp_check_drc_props_v2(struct device_node
*dn, char *drc_name,
int rpaphp_check_drc_props(struct device_node *dn, char *drc_name,
char *drc_type)
{
- const unsigned int *my_index;
+ const __be32 *my_index;
my_index = of_get_property(dn, "ibm,my-drc-index", NULL);
if (!my_index) {
@@ -277,10 +277,10 @@ int rpaphp_check_drc_props(struct device_node *dn, char
*drc_name,
if (of_find_property(dn->parent, "ibm,drc-info", NULL))
return rpaphp_check_drc_props_v2(dn, drc_name, drc_type,
- *my_index);
+ be32_to_cpu(*my_index));
else
return rpaphp_check_drc_props_v1(dn, drc_name, drc_type,
- *my_index);
+ be32_to_cpu(*my_index));
}
EXPORT_SYMBOL_GPL(rpaphp_check_drc_props);
@@ -311,10 +311,10 @@ static int is_php_type(char *drc_type)
* for built-in pci slots (even when the built-in slots are
* dlparable.)
*/
-static int is_php_dn(struct device_node *dn, const int **indexes,
- const int **names, const int **types, const int **power_domains)
+static int is_php_dn(struct device_node *dn, const __be32 **indexes,
+ const __be32 **names, const __be32 **types, const __be32
**power_domains)
{
- const int *drc_types;
+ const __be32 *drc_types;
int rc;
rc = get_children_props(dn, indexes, names, &drc_types, power_domains);
@@ -374,7 +374,7 @@ static int rpaphp_drc_add_slot(struct device_node *dn)
struct slot *slot;
int retval = 0;
int i;
- const int *indexes, *names, *types, *power_domains;
+ const __be32 *indexes, *names, *types, *power_domains;
char *name, *type;
/* If this is not a hotplug slot, return without doing anything. */
--
2.7.4
[RFC PATCH 4/9] PCI: rpaphp: fix up pointer to first drc-info entry
The first entry of the ibm,drc-info property is an int encoded count
of the number of drc-info entries that follow. The "value" pointer
returned by of_prop_next_u32() is still pointing at the this value
when we call of_read_drc_info_cell().
Fix up by incrementing the "value" pointer to point at the first
element of the first drc-info entry prior.
Signed-off-by: Tyrel Datwyler
---
drivers/pci/hotplug/rpaphp_core.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/pci/hotplug/rpaphp_core.c
b/drivers/pci/hotplug/rpaphp_core.c
index 18627bb..e350264 100644
--- a/drivers/pci/hotplug/rpaphp_core.c
+++ b/drivers/pci/hotplug/rpaphp_core.c
@@ -239,6 +239,8 @@ static int rpaphp_check_drc_props_v2(struct device_node
*dn, char *drc_name,
value = of_prop_next_u32(info, NULL, &entries);
if (!value)
return -EINVAL;
+ else
+ value++;
for (j = 0; j < entries; j++) {
of_read_drc_info_cell(&info, &value, &drc);
--
2.7.4
[RFC PATCH 5/9] PCI: rpaphp: don't rely on firmware feature to imply drc-info support
In the event that the partition is migrated to a platform with older firmware that doesn't support the ibm,drc-info property the device tree is modified to remove the ibm,drc-info property and replace it with the older style ibm,drc-* properties for types, names, indexes, and power-domains. One of the requirements of the drc-info firmware feature is the the client is able to handle both the new property, and old properties. Therefore we can't rely on the firmware feature alone to dictate which property is currently present in the device tree. Fix this short coming by checking explicitly for the ibm,drc-info property, and falling back to the older ibm,drc-* properties if it doesn't exist. Signed-off-by: Tyrel Datwyler --- drivers/pci/hotplug/rpaphp_core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/pci/hotplug/rpaphp_core.c b/drivers/pci/hotplug/rpaphp_core.c index e350264..e18e9a0 100644 --- a/drivers/pci/hotplug/rpaphp_core.c +++ b/drivers/pci/hotplug/rpaphp_core.c @@ -275,7 +275,7 @@ int rpaphp_check_drc_props(struct device_node *dn, char *drc_name, return -EINVAL; } - if (firmware_has_feature(FW_FEATURE_DRC_INFO)) + if (of_find_property(dn->parent, "ibm,drc-info", NULL)) return rpaphp_check_drc_props_v2(dn, drc_name, drc_type, *my_index); else -- 2.7.4
[RFC PATCH 2/9] powerpc/pseries: fix bad drc_index_start value parsing of drc-info entry
The ibm,drc-info property is an array property that contains drc-info
entries such that each entry is made up of 2 string encoded elements
followed by 5 int encoded elements. The of_read_drc_info_cell()
helper contains comments that correctly name the expected elements
and their encoding. However, the usage of of_prop_next_string() and
of_prop_next_u32() introduced a subtle skippage of the first u32.
This is a result of of_prop_next_string() returns a pointer to the
next property value which is not a string, but actually a (__be32 *).
As, a result the following call to of_prop_next_u32() passes over the
current int encoded value and actually stores the next one wrongly.
Simply endian swap the current value in place after reading the first
two string values. The remaining int encoded values can then be read
correctly using of_prop_next_u32().
Signed-off-by: Tyrel Datwyler
---
arch/powerpc/platforms/pseries/of_helpers.c | 8 +++-
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/of_helpers.c
b/arch/powerpc/platforms/pseries/of_helpers.c
index 6df192f..66dfd82 100644
--- a/arch/powerpc/platforms/pseries/of_helpers.c
+++ b/arch/powerpc/platforms/pseries/of_helpers.c
@@ -45,14 +45,14 @@ struct device_node *pseries_of_derive_parent(const char
*path)
int of_read_drc_info_cell(struct property **prop, const __be32 **curval,
struct of_drc_info *data)
{
- const char *p;
+ const char *p = (char *)(*curval);
const __be32 *p2;
if (!data)
return -EINVAL;
/* Get drc-type:encode-string */
- p = data->drc_type = (char*) (*curval);
+ data->drc_type = (char *)p;
p = of_prop_next_string(*prop, p);
if (!p)
return -EINVAL;
@@ -65,9 +65,7 @@ int of_read_drc_info_cell(struct property **prop, const
__be32 **curval,
/* Get drc-index-start:encode-int */
p2 = (const __be32 *)p;
- p2 = of_prop_next_u32(*prop, p2, &data->drc_index_start);
- if (!p2)
- return -EINVAL;
+ data->drc_index_start = be32_to_cpu(*p2);
/* Get drc-name-suffix-start:encode-int */
p2 = of_prop_next_u32(*prop, p2, &data->drc_name_suffix_start);
--
2.7.4
[RFC PATCH 3/9] powerpc/pseries: fix drc-info mappings of logical cpus to drc-index
There are a couple subtle errors in the mapping between cpu-ids and a
cpus associated drc-index when using the new ibm,drc-info property.
The first is we while drc-info may have been a supported firmware
feature at boot it is possible we have migrated to a CEC with older
firmware that doesn't support the ibm,drc-info property. In that case
the device tree would have been updated after migration to remove the
ibm,drc-info property and replace it with the older style ibm,drc-*
properties for types, indexes, names, and power-domains.
The second is that the first value of the ibm,drc-info property is
the int encoded count of drc-info entries. As such "value" returned
by of_prop_next_u32() is pointing at that count, and not the first
element of the first drc-info entry as is expected by the
of_read_drc_info_cell() helper.
Fix the first by ignoring DRC-INFO firmware feature and instead
testing directly for ibm,drc-info, and then falling back to the
old style ibm,drc-indexes in the case it doesn't exit.
Fix the second by incrementing value to the next element prior to
parsing drc-info entries.
Signed-off-by: Tyrel Datwyler
---
arch/powerpc/platforms/pseries/pseries_energy.c | 23 ++-
1 file changed, 10 insertions(+), 13 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/pseries_energy.c
b/arch/powerpc/platforms/pseries/pseries_energy.c
index a96874f..09e98d3 100644
--- a/arch/powerpc/platforms/pseries/pseries_energy.c
+++ b/arch/powerpc/platforms/pseries/pseries_energy.c
@@ -36,6 +36,7 @@ static int sysfs_entries;
static u32 cpu_to_drc_index(int cpu)
{
struct device_node *dn = NULL;
+ struct property *info;
int thread_index;
int rc = 1;
u32 ret = 0;
@@ -47,20 +48,18 @@ static u32 cpu_to_drc_index(int cpu)
/* Convert logical cpu number to core number */
thread_index = cpu_core_index_of_thread(cpu);
- if (firmware_has_feature(FW_FEATURE_DRC_INFO)) {
- struct property *info = NULL;
+ info = of_find_property(dn, "ibm,drc-info", NULL);
+ if (info) {
struct of_drc_info drc;
int j;
u32 num_set_entries;
const __be32 *value;
- info = of_find_property(dn, "ibm,drc-info", NULL);
- if (info == NULL)
- goto err_of_node_put;
-
value = of_prop_next_u32(info, NULL, &num_set_entries);
if (!value)
goto err_of_node_put;
+ else
+ value++;
for (j = 0; j < num_set_entries; j++) {
@@ -110,6 +109,7 @@ static u32 cpu_to_drc_index(int cpu)
static int drc_index_to_cpu(u32 drc_index)
{
struct device_node *dn = NULL;
+ struct property *info;
const int *indexes;
int thread_index = 0, cpu = 0;
int rc = 1;
@@ -117,21 +117,18 @@ static int drc_index_to_cpu(u32 drc_index)
dn = of_find_node_by_path("/cpus");
if (dn == NULL)
goto err;
-
- if (firmware_has_feature(FW_FEATURE_DRC_INFO)) {
- struct property *info = NULL;
+ info = of_find_property(dn, "ibm,drc-info", NULL);
+ if (info) {
struct of_drc_info drc;
int j;
u32 num_set_entries;
const __be32 *value;
- info = of_find_property(dn, "ibm,drc-info", NULL);
- if (info == NULL)
- goto err_of_node_put;
-
value = of_prop_next_u32(info, NULL, &num_set_entries);
if (!value)
goto err_of_node_put;
+ else
+ value++;
for (j = 0; j < num_set_entries; j++) {
--
2.7.4
[RFC PATCH 1/9] powerpc/pseries: add cpu DLPAR support for drc-info property
From: Tyrel Datwyler
Older firmwares provided information about Dynamic Reconfig
Connectors (DRC) through several device tree properties, namely
ibm,drc-types, ibm,drc-indexes, ibm,drc-names, and
ibm,drc-power-domains. New firmwares have the ability to present this
same information in a much condensed format through a device tree
property called ibm,drc-info.
The existing cpu DLPAR hotplug code only understands the older DRC
property format when validating the drc-index of a cpu during a
hotplug add. This updates those code paths to use the ibm,drc-info
property, when present, instead for validation.
Signed-off-by: Tyrel Datwyler
---
arch/powerpc/platforms/pseries/hotplug-cpu.c | 117 ---
1 file changed, 89 insertions(+), 28 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c
b/arch/powerpc/platforms/pseries/hotplug-cpu.c
index bbda646..a2b6cd1 100644
--- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
+++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
@@ -407,25 +407,61 @@ static bool dlpar_cpu_exists(struct device_node *parent,
u32 drc_index)
return found;
}
-static bool valid_cpu_drc_index(struct device_node *parent, u32 drc_index)
+static bool drc_info_valid_index(struct device_node *parent, u32 drc_index)
{
- bool found = false;
- int rc, index;
+ struct property *info;
+ struct of_drc_info drc;
+ const __be32 *value;
+ int count, i, j;
- index = 0;
- while (!found) {
- u32 drc;
+ info = of_find_property(parent, "ibm,drc-info", NULL);
+ if (!info)
+ return false;
- rc = of_property_read_u32_index(parent, "ibm,drc-indexes",
- index++, &drc);
- if (rc)
+ value = of_prop_next_u32(info, NULL, &count);
+
+ /* First value of ibm,drc-info is number of drc-info records */
+ if (value)
+ value++;
+ else
+ return false;
+
+ for (i = 0; i < count; i++) {
+ if (of_read_drc_info_cell(&info, &value, &drc))
+ return false;
+
+ if (strncmp(drc.drc_type, "CPU", 3))
break;
- if (drc == drc_index)
- found = true;
+ if (drc_index > drc.last_drc_index)
+ continue;
+
+ for (j = 0; j < drc.num_sequential_elems; j++)
+ if (drc_index == (drc.drc_index_start +
(drc.sequential_inc * j)))
+ return true;
}
- return found;
+ return false;
+}
+
+static bool valid_cpu_drc_index(struct device_node *parent, u32 drc_index)
+{
+ const __be32 *indexes;
+ int i;
+
+ if (of_find_property(parent, "ibm,drc-info", NULL))
+ return drc_info_valid_index(parent, drc_index);
+
+ indexes = of_get_property(parent, "ibm,drc-indexes", NULL);
+ if (!indexes)
+ return false;
+
+ for (i = 0; i < indexes[0]; i++) {
+ if (be32_to_cpu(indexes[i + 1]) == drc_index)
+ return true;
+ }
+
+ return false;
}
static ssize_t dlpar_cpu_add(u32 drc_index)
@@ -720,8 +756,11 @@ static int dlpar_cpu_remove_by_count(u32 cpus_to_remove)
static int find_dlpar_cpus_to_add(u32 *cpu_drcs, u32 cpus_to_add)
{
struct device_node *parent;
+ struct property *info;
+ const __be32 *indexes;
int cpus_found = 0;
- int index, rc;
+ int i, j;
+ u32 drc_index;
parent = of_find_node_by_path("/cpus");
if (!parent) {
@@ -730,24 +769,46 @@ static int find_dlpar_cpus_to_add(u32 *cpu_drcs, u32
cpus_to_add)
return -1;
}
- /* Search the ibm,drc-indexes array for possible CPU drcs to
-* add. Note that the format of the ibm,drc-indexes array is
-* the number of entries in the array followed by the array
-* of drc values so we start looking at index = 1.
-*/
- index = 1;
- while (cpus_found < cpus_to_add) {
- u32 drc;
+ info = of_find_property(parent, "ibm,drc-info", NULL);
+ if (info) {
+ struct of_drc_info drc;
+ const __be32 *value;
+ int count;
- rc = of_property_read_u32_index(parent, "ibm,drc-indexes",
- index++, &drc);
- if (rc)
- break;
+ value = of_prop_next_u32(info, NULL, &count);
+ if (value)
+ value++;
- if (dlpar_cpu_exists(parent, drc))
- continue;
+ for (i = 0; i < count; i++) {
+ of_read_drc_info
Re: [PATCH] net/ibmvnic: Fix missing { in __ibmvnic_reset
On 9/9/19 1:44 PM, Michal Suchanek wrote:
> Commit 1c2977c09499 ("net/ibmvnic: free reset work of removed device from
> queue")
> adds a } without corresponding { causing build break.
>
> Fixes: 1c2977c09499 ("net/ibmvnic: free reset work of removed device from
> queue")
> Signed-off-by: Michal Suchanek
Reviewed-by: Tyrel Datwyler
> ---
> drivers/net/ethernet/ibm/ibmvnic.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/ibm/ibmvnic.c
> b/drivers/net/ethernet/ibm/ibmvnic.c
> index 6644cabc8e75..5cb55ea671e3 100644
> --- a/drivers/net/ethernet/ibm/ibmvnic.c
> +++ b/drivers/net/ethernet/ibm/ibmvnic.c
> @@ -1984,7 +1984,7 @@ static void __ibmvnic_reset(struct work_struct *work)
> rwi = get_next_rwi(adapter);
> while (rwi) {
> if (adapter->state == VNIC_REMOVING ||
> - adapter->state == VNIC_REMOVED)
> + adapter->state == VNIC_REMOVED) {
> kfree(rwi);
> rc = EBUSY;
> break;
>
Re: [PATCH] PCI: hotplug: Remove surplus return from a void function
On 8/26/19 2:51 AM, Krzysztof Wilczynski wrote:
> Remove unnecessary empty return statement at the end of a void
> function in the following:
>
> - drivers/pci/hotplug/cpci_hotplug_core.c: cleanup_slots()
> - drivers/pci/hotplug/cpqphp_core.c: pci_print_IRQ_route()
> - drivers/pci/hotplug/cpqphp_ctrl.c: cpqhp_pushbutton_thread()
> - drivers/pci/hotplug/cpqphp_ctrl.c: interrupt_event_handler()
> - drivers/pci/hotplug/cpqphp_nvram.h: compaq_nvram_init()
> - drivers/pci/hotplug/rpadlpar_core.c: rpadlpar_io_init()
> - drivers/pci/hotplug/rpaphp_core.c: cleanup_slots()
>
> Signed-off-by: Krzysztof Wilczynski
For rpa*_core.c portions,
Acked-by: Tyrel Datwyler
> ---
> drivers/pci/hotplug/cpci_hotplug_core.c | 1 -
> drivers/pci/hotplug/cpqphp_core.c | 1 -
> drivers/pci/hotplug/cpqphp_ctrl.c | 4
> drivers/pci/hotplug/cpqphp_nvram.h | 5 +
> drivers/pci/hotplug/rpadlpar_core.c | 1 -
> drivers/pci/hotplug/rpaphp_core.c | 1 -
> 6 files changed, 1 insertion(+), 12 deletions(-)
>
> diff --git a/drivers/pci/hotplug/cpci_hotplug_core.c
> b/drivers/pci/hotplug/cpci_hotplug_core.c
> index 603eadf3d965..d0559d2faf50 100644
> --- a/drivers/pci/hotplug/cpci_hotplug_core.c
> +++ b/drivers/pci/hotplug/cpci_hotplug_core.c
> @@ -563,7 +563,6 @@ cleanup_slots(void)
> }
> cleanup_null:
> up_write(&list_rwsem);
> - return;
> }
>
> int
> diff --git a/drivers/pci/hotplug/cpqphp_core.c
> b/drivers/pci/hotplug/cpqphp_core.c
> index 16bbb183695a..b8aacb41a83c 100644
> --- a/drivers/pci/hotplug/cpqphp_core.c
> +++ b/drivers/pci/hotplug/cpqphp_core.c
> @@ -173,7 +173,6 @@ static void pci_print_IRQ_route(void)
> dbg("%d %d %d %d\n", tbus, tdevice >> 3, tdevice & 0x7, tslot);
>
> }
> - return;
> }
>
>
> diff --git a/drivers/pci/hotplug/cpqphp_ctrl.c
> b/drivers/pci/hotplug/cpqphp_ctrl.c
> index b7f4e1f099d9..68de958a9be8 100644
> --- a/drivers/pci/hotplug/cpqphp_ctrl.c
> +++ b/drivers/pci/hotplug/cpqphp_ctrl.c
> @@ -1872,8 +1872,6 @@ static void interrupt_event_handler(struct controller
> *ctrl)
> }
> } /* End of FOR loop */
> }
> -
> - return;
> }
>
>
> @@ -1943,8 +1941,6 @@ void cpqhp_pushbutton_thread(struct timer_list *t)
>
> p_slot->state = STATIC_STATE;
> }
> -
> - return;
> }
>
>
> diff --git a/drivers/pci/hotplug/cpqphp_nvram.h
> b/drivers/pci/hotplug/cpqphp_nvram.h
> index 918ff8dbfe62..70e879b6a23f 100644
> --- a/drivers/pci/hotplug/cpqphp_nvram.h
> +++ b/drivers/pci/hotplug/cpqphp_nvram.h
> @@ -16,10 +16,7 @@
>
> #ifndef CONFIG_HOTPLUG_PCI_COMPAQ_NVRAM
>
> -static inline void compaq_nvram_init(void __iomem *rom_start)
> -{
> - return;
> -}
> +static inline void compaq_nvram_init(void __iomem *rom_start) { }
>
> static inline int compaq_nvram_load(void __iomem *rom_start, struct
> controller *ctrl)
> {
> diff --git a/drivers/pci/hotplug/rpadlpar_core.c
> b/drivers/pci/hotplug/rpadlpar_core.c
> index 182f9e3443ee..977946e4e613 100644
> --- a/drivers/pci/hotplug/rpadlpar_core.c
> +++ b/drivers/pci/hotplug/rpadlpar_core.c
> @@ -473,7 +473,6 @@ int __init rpadlpar_io_init(void)
> void rpadlpar_io_exit(void)
> {
> dlpar_sysfs_exit();
> - return;
> }
>
> module_init(rpadlpar_io_init);
> diff --git a/drivers/pci/hotplug/rpaphp_core.c
> b/drivers/pci/hotplug/rpaphp_core.c
> index c3899ee1db99..18627bb21e9e 100644
> --- a/drivers/pci/hotplug/rpaphp_core.c
> +++ b/drivers/pci/hotplug/rpaphp_core.c
> @@ -408,7 +408,6 @@ static void __exit cleanup_slots(void)
> pci_hp_deregister(&slot->hotplug_slot);
> dealloc_slot_struct(slot);
> }
> - return;
> }
>
> static int __init rpaphp_init(void)
>
Re: [PATCH v2 1/3] powerpc/rtas: use device model APIs and serialization during LPM
On 8/2/19 12:29 PM, Nathan Lynch wrote:
> The LPAR migration implementation and userspace-initiated cpu hotplug
> can interleave their executions like so:
>
> 1. Set cpu 7 offline via sysfs.
>
> 2. Begin a partition migration, whose implementation requires the OS
>to ensure all present cpus are online; cpu 7 is onlined:
>
> rtas_ibm_suspend_me -> rtas_online_cpus_mask -> cpu_up
>
>This sets cpu 7 online in all respects except for the cpu's
>corresponding struct device; dev->offline remains true.
>
> 3. Set cpu 7 online via sysfs. _cpu_up() determines that cpu 7 is
>already online and returns success. The driver core (device_online)
>sets dev->offline = false.
>
> 4. The migration completes and restores cpu 7 to offline state:
>
> rtas_ibm_suspend_me -> rtas_offline_cpus_mask -> cpu_down
>
> This leaves cpu7 in a state where the driver core considers the cpu
> device online, but in all other respects it is offline and
> unused. Attempts to online the cpu via sysfs appear to succeed but the
> driver core actually does not pass the request to the lower-level
> cpuhp support code. This makes the cpu unusable until the cpu device
> is manually set offline and then online again via sysfs.
>
> Instead of directly calling cpu_up/cpu_down, the migration code should
> use the higher-level device core APIs to maintain consistent state and
> serialize operations.
>
> Fixes: 120496ac2d2d ("powerpc: Bring all threads online prior to
> migration/hibernation")
> Signed-off-by: Nathan Lynch
> ---
> arch/powerpc/kernel/rtas.c | 11 ---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
> index 5faf0a64c92b..05824eb4323b 100644
> --- a/arch/powerpc/kernel/rtas.c
> +++ b/arch/powerpc/kernel/rtas.c
> @@ -871,15 +871,17 @@ static int rtas_cpu_state_change_mask(enum
> rtas_cpu_state state,
> return 0;
>
> for_each_cpu(cpu, cpus) {
> + struct device *dev = get_cpu_device(cpu);
> +
> switch (state) {
> case DOWN:
> - cpuret = cpu_down(cpu);
> + cpuret = device_offline(dev);
> break;
> case UP:
> - cpuret = cpu_up(cpu);
> + cpuret = device_online(dev);
Not that I have a problem with using the core device api, but as an FYI we had
discussed in the past introducing one shot functions in kernel/cpu.c for doing
our take down, bring up where cpu_update_maps() can be held for the whole
process. The thought was maybe it would be useful generically being able to
online/offline a bulk subset.
> break;
> }
> - if (cpuret) {
> + if (cpuret < 0) {
> pr_debug("%s: cpu_%s for cpu#%d returned %d.\n",
> __func__,
> ((state == UP) ? "up" : "down"),
> @@ -968,6 +970,8 @@ int rtas_ibm_suspend_me(u64 handle)
> data.token = rtas_token("ibm,suspend-me");
> data.complete = &done;
>
> + lock_device_hotplug();
> +
Does taking the device hotplug lock suffice to prevent races with sysfs attempts
to hotplug (on/off) cpus? And if so we can strip out the code that checks the
mask to see if we raced, correct?
-Tyrel
> /* All present CPUs must be online */
> cpumask_andnot(offline_mask, cpu_present_mask, cpu_online_mask);
> cpuret = rtas_online_cpus_mask(offline_mask);
> @@ -1006,6 +1010,7 @@ int rtas_ibm_suspend_me(u64 handle)
> __func__);
>
> out:
> + unlock_device_hotplug();
> free_cpumask_var(offline_mask);
> return atomic_read(&data.error);
> }
>
Re: [PATCH] scsi: ibmvfc: Mark expected switch fall-throughs
On 7/28/19 5:26 PM, Gustavo A. R. Silva wrote: > Mark switch cases where we are expecting to fall through. > > This patch fixes the following warnings: > > drivers/scsi/ibmvscsi/ibmvfc.c: In function 'ibmvfc_npiv_login_done': > drivers/scsi/ibmvscsi/ibmvfc.c:4022:3: warning: this statement may fall > through [-Wimplicit-fallthrough=] >ibmvfc_retry_host_init(vhost); >^ > drivers/scsi/ibmvscsi/ibmvfc.c:4023:2: note: here > case IBMVFC_MAD_DRIVER_FAILED: > ^~~~ > drivers/scsi/ibmvscsi/ibmvfc.c: In function 'ibmvfc_bsg_request': > drivers/scsi/ibmvscsi/ibmvfc.c:1830:11: warning: this statement may fall > through [-Wimplicit-fallthrough=] >port_id = (bsg_request->rqst_data.h_els.port_id[0] << 16) | >^~~ > (bsg_request->rqst_data.h_els.port_id[1] << 8) | > > bsg_request->rqst_data.h_els.port_id[2]; > ~~~ > drivers/scsi/ibmvscsi/ibmvfc.c:1833:2: note: here > case FC_BSG_RPT_ELS: > ^~~~ > drivers/scsi/ibmvscsi/ibmvfc.c:1838:11: warning: this statement may fall > through [-Wimplicit-fallthrough=] >port_id = (bsg_request->rqst_data.h_ct.port_id[0] << 16) | >^~ > (bsg_request->rqst_data.h_ct.port_id[1] << 8) | > ~~~ > bsg_request->rqst_data.h_ct.port_id[2]; > ~~ > drivers/scsi/ibmvscsi/ibmvfc.c:1841:2: note: here > case FC_BSG_RPT_CT: > ^~~~ > > Reported-by: Stephen Rothwell > Signed-off-by: Gustavo A. R. Silva > --- Acked-by: Tyrel Datwyler
[PATCH] ibmvfc: fix WARN_ON during event pool release
While removing an ibmvfc client adapter a WARN_ON like the following WARN_ON is seen in the kernel log: WARNING: CPU: 6 PID: 5421 at ./include/linux/dma-mapping.h:541 ibmvfc_free_event_pool+0x12c/0x1f0 [ibmvfc] CPU: 6 PID: 5421 Comm: rmmod Tainted: GE 4.17.0-rc1-next-20180419-autotest #1 NIP: d290328c LR: d290325c CTR: c036ee20 REGS: c00288d1b7e0 TRAP: 0700 Tainted: GE (4.17.0-rc1-next-20180419-autotest) MSR: 80010282b033 CR: 44008828 XER: 2000 CFAR: c036e408 SOFTE: 1 GPR00: d290325c c00288d1ba60 d2917900 c00289d75448 GPR04: 0071 c000ff87 1804 0001 GPR08: c156e838 0001 d290c640 GPR12: c036ee20 c0001ec4dc00 GPR16: 0100276901e0 10020598 GPR20: 10020550 10020538 10020578 100205b0 GPR24: 10020590 5deadbeef100 GPR28: 5deadbeef200 d2910b00 0071 c002822f87d8 NIP [d290328c] ibmvfc_free_event_pool+0x12c/0x1f0 [ibmvfc] LR [d290325c] ibmvfc_free_event_pool+0xfc/0x1f0 [ibmvfc] Call Trace: [c00288d1ba60] [d290325c] ibmvfc_free_event_pool+0xfc/0x1f0 [ibmvfc] (unreliable) [c00288d1baf0] [d2909390] ibmvfc_abort_task_set+0x7b0/0x8b0 [ibmvfc] [c00288d1bb70] [c00d8c68] vio_bus_remove+0x68/0x100 [c00288d1bbb0] [c07da7c4] device_release_driver_internal+0x1f4/0x2d0 [c00288d1bc00] [c07da95c] driver_detach+0x7c/0x100 [c00288d1bc40] [c07d8af4] bus_remove_driver+0x84/0x140 [c00288d1bcb0] [c07db6ac] driver_unregister+0x4c/0xa0 [c00288d1bd20] [c00d6e7c] vio_unregister_driver+0x2c/0x50 [c00288d1bd50] [d290ba0c] cleanup_module+0x24/0x15e0 [ibmvfc] [c00288d1bd70] [c01dadb0] sys_delete_module+0x220/0x2d0 [c00288d1be30] [c000b284] system_call+0x58/0x6c Instruction dump: e8410018 e87f0068 809f0078 e8bf0080 e8df0088 2fa3 419e008c e9230200 2fa9 419e0080 894d098a 794a07e0 <0b0a> e9290008 2fa9 419e0028 This is tripped as a result of irqs being disabled during the call to dma_free_coherent() by ibmvfc_free_event_pool(). At this point in the code path we have quiesced the adapter and its overly paranoid anyways to be holding the host lock. Reported-by: Abdul Haleem Signed-off-by: Tyrel Datwyler --- drivers/scsi/ibmvscsi/ibmvfc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c index acd16e0d52cf..8cdbac076a1b 100644 --- a/drivers/scsi/ibmvscsi/ibmvfc.c +++ b/drivers/scsi/ibmvscsi/ibmvfc.c @@ -4864,8 +4864,8 @@ static int ibmvfc_remove(struct vio_dev *vdev) spin_lock_irqsave(vhost->host->host_lock, flags); ibmvfc_purge_requests(vhost, DID_ERROR); - ibmvfc_free_event_pool(vhost); spin_unlock_irqrestore(vhost->host->host_lock, flags); + ibmvfc_free_event_pool(vhost); ibmvfc_free_mem(vhost); spin_lock(&ibmvfc_driver_lock); -- 2.12.3
Re: Coccinelle: Checking of_node_put() calls with SmPL
On 07/10/2019 11:35 PM, wen.yan...@zte.com.cn wrote:
>>> we developed a coccinelle script to detect such problems.
>>
>> Would you find the implementation of the function “dt_init_idle_driver”
>> suspicious according to discussed source code search patterns?
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/cpuidle/dt_idle_states.c?id=e9a83bd2322035ed9d7dcf35753d3f984d76c6a5#n208
>> https://elixir.bootlin.com/linux/v5.2/source/drivers/cpuidle/dt_idle_states.c#L208
>>
>>
>>> This script is still being improved.
>>
>> Will corresponding software development challenges become more interesting?
>
> Hello Markus,
> This is the simplified code pattern for it:
>
> 172 for (i = 0; ; i++) {
This loop can only be exited on a break.
> 173 state_node = of_parse_phandle(...); ---> Obtain here
> ...
> 177 match_id = of_match_node(matches, state_node);
> 178 if (!match_id) {
> 179 err = -ENODEV;
> 180 break; ---> Jump out of
> the loop without releasing it
> 181 }
> 182
> 183 if (!of_device_is_available(state_node)) {
> 184 of_node_put(state_node);
> 185 continue;---> Release the
> object references within a loop
> 186 }
> ...
> 208 of_node_put(state_node); --> Release the object
> references within a loop
This is required at the end of every loop or continue to free the reference.
Only a break will exit the loop where we hit the below of_node_put().
> 209 }
> 210
> 211 of_node_put(state_node); -->There may be double free
> here.
None of the break conditions call of_node_put(), so it needs to be called here.
-Tyrel
>
> This code pattern is very interesting and the coccinelle software should also
> recognize this pattern.
>
> Regards,
> Wen
>
Re: [PATCH] powerpc/pseries: fix oops in hotplug memory notifier
On 06/13/2019 06:05 PM, Nathan Lynch wrote: > Nathan Lynch writes: > >> Tyrel Datwyler writes: >> >>> Maybe we are ok with this behavior as I haven't dug deep enough into the >>> memory >>> subsystem here to really understand what the memory code is updating, but >>> it is >>> concerning that we are doing it in some cases, but not all. >> >> I hope I've made a good case above that the notifier does not do any >> useful work, and a counterpart for the v2 format isn't needed. Do you >> agree? >> >> If so, I'll send a patch later to remove the notifier altogether. In the >> near term I would still like this minimal fix to go in. > > Tyrel, ack? > Sure, lets start with simple case to fix the crash. -Tyrel
Re: [PATCH] powerpc/pseries: fix oops in hotplug memory notifier
On 06/06/2019 10:04 PM, Nathan Lynch wrote: > During post-migration device tree updates, we can oops in > pseries_update_drconf_memory if the source device tree has an > ibm,dynamic-memory-v2 property and the destination has a > ibm,dynamic_memory (v1) property. The notifier processes an "update" > for the ibm,dynamic-memory property but it's really an add in this > scenario. So make sure the old property object is there before > dereferencing it. > > Signed-off-by: Nathan Lynch > --- Ok, so I think there are a couple things to address here in regards to the nice mess PFW has gotten us into. Yes, this patch solves the oops, but I worry it just papers over some short comings in our code. After some poking around unless I'm mistaken our memory notifier only handles v1 versions of the ibm,dynamic-memory property. So, on newer firmware we aren't doing any memory fixups if v2 (ibm,dynamic-memory-v2) of the property is updated. For older PFW if we have source and target that only support v1 we will update the memory in response to any update to ibm,dynamic-memory. It also appears to be the case if we start with v1 and migrate to a target with newer PFW that supports both v1 and v2 that the PFW will continue with v1 on the target and as a result we update memory in accordance to a property update to ibm,dynamic-memory. Now, if we have source and targets that both support v2 after a migration we will do no update in response to ibm,dynamic-memory-v2 changes. And then there is the case of a source with v2 support migrating to a target with only v1 support where we observe this oops. The oops is a result of ibm,dynamic-memory being a new property that is added and there for no old property date exists. However, simply returning in response also has the side effect that we do not update memory in response to a device tree update of dynamic memory. Maybe we are ok with this behavior as I haven't dug deep enough into the memory subsystem here to really understand what the memory code is updating, but it is concerning that we are doing it in some cases, but not all. -Tyrel > arch/powerpc/platforms/pseries/hotplug-memory.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c > b/arch/powerpc/platforms/pseries/hotplug-memory.c > index 47087832f8b2..e6bd172bcf30 100644 > --- a/arch/powerpc/platforms/pseries/hotplug-memory.c > +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c > @@ -980,6 +980,9 @@ static int pseries_update_drconf_memory(struct > of_reconfig_data *pr) > if (!memblock_size) > return -EINVAL; > > + if (!pr->old_prop) > + return 0; > + > p = (__be32 *) pr->old_prop->value; > if (!p) > return -EINVAL; >
Re: [PATCH] powerpc/setup_64: fix -Wempty-body warnings
On 06/05/2019 01:17 PM, Qian Cai wrote:
> At the beginning of setup_64.c, it has,
>
> #ifdef DEBUG
> #define DBG(fmt...) udbg_printf(fmt)
> #else
> #define DBG(fmt...)
> #endif
Simpler solution is just to define the debug in the else clause as such:
#define DBG(fmt...) do { } while(0)
-Tyrel
>
> where DBG() could be compiled away, and generate warnings,
>
> arch/powerpc/kernel/setup_64.c: In function 'initialize_cache_info':
> arch/powerpc/kernel/setup_64.c:579:49: warning: suggest braces around
> empty body in an 'if' statement [-Wempty-body]
> DBG("Argh, can't find dcache properties !\n");
> ^
> arch/powerpc/kernel/setup_64.c:582:49: warning: suggest braces around
> empty body in an 'if' statement [-Wempty-body]
> DBG("Argh, can't find icache properties !\n");
>
> Signed-off-by: Qian Cai
> ---
> arch/powerpc/kernel/setup_64.c | 9 +
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
> index 44b4c432a273..23758834324f 100644
> --- a/arch/powerpc/kernel/setup_64.c
> +++ b/arch/powerpc/kernel/setup_64.c
> @@ -575,12 +575,13 @@ void __init initialize_cache_info(void)
>* d-cache and i-cache sizes... -Peter
>*/
> if (cpu) {
> - if (!parse_cache_info(cpu, false, &ppc64_caches.l1d))
> + /* Add an extra brace to silence -Wempty-body warnings. */
> + if (!parse_cache_info(cpu, false, &ppc64_caches.l1d)) {
> DBG("Argh, can't find dcache properties !\n");
> -
> - if (!parse_cache_info(cpu, true, &ppc64_caches.l1i))
> + }
> + if (!parse_cache_info(cpu, true, &ppc64_caches.l1i)) {
> DBG("Argh, can't find icache properties !\n");
> -
> + }
> /*
>* Try to find the L2 and L3 if any. Assume they are
>* unified and use the D-side properties.
>
Re: [PATCH v2] PCI: rpaphp: Avoid a sometimes-uninitialized warning
On 06/03/2019 03:11 PM, Nathan Chancellor wrote:
> When building with -Wsometimes-uninitialized, clang warns:
>
> drivers/pci/hotplug/rpaphp_core.c:243:14: warning: variable 'fndit' is
> used uninitialized whenever 'for' loop exits because its condition is
> false [-Wsometimes-uninitialized]
> for (j = 0; j < entries; j++) {
> ^~~
> drivers/pci/hotplug/rpaphp_core.c:256:6: note: uninitialized use occurs
> here
> if (fndit)
> ^
> drivers/pci/hotplug/rpaphp_core.c:243:14: note: remove the condition if
> it is always true
> for (j = 0; j < entries; j++) {
> ^~~
> drivers/pci/hotplug/rpaphp_core.c:233:14: note: initialize the variable
> 'fndit' to silence this warning
> int j, fndit;
> ^
> = 0
>
> fndit is only used to gate a sprintf call, which can be moved into the
> loop to simplify the code and eliminate the local variable, which will
> fix this warning.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/504
> Fixes: 2fcf3ae508c2 ("hotplug/drc-info: Add code to search ibm,drc-info
> property")
> Suggested-by: Nick Desaulniers
> Signed-off-by: Nathan Chancellor
Acked-by: Tyrel Datwyler
Re: [PATCH v3] scsi: ibmvscsi: Don't use rc uninitialized in ibmvscsi_do_work
On 06/03/2019 04:44 PM, Nathan Chancellor wrote:
> clang warns:
>
> drivers/scsi/ibmvscsi/ibmvscsi.c:2126:7: warning: variable 'rc' is used
> uninitialized whenever switch case is taken [-Wsometimes-uninitialized]
> case IBMVSCSI_HOST_ACTION_NONE:
> ^
> drivers/scsi/ibmvscsi/ibmvscsi.c:2151:6: note: uninitialized use occurs
> here
> if (rc) {
> ^~
>
> Initialize rc in the IBMVSCSI_HOST_ACTION_UNBLOCK case statement then
> shuffle IBMVSCSI_HOST_ACTION_NONE down to the default case statement and
> make it return early so that rc is never used uninitialized in this
> function.
>
> Fixes: 035a3c4046b5 ("scsi: ibmvscsi: redo driver work thread to use enum
> action states")
> Link: https://github.com/ClangBuiltLinux/linux/issues/502
> Suggested-by: Michael Ellerman
> Suggested-by: Tyrel Datwyler
> Signed-off-by: Nathan Chancellor
Acked-by: Tyrel Datwyler
Re: [PATCH v2] scsi: ibmvscsi: Don't use rc uninitialized in ibmvscsi_do_work
On 06/03/2019 04:34 PM, Tyrel Datwyler wrote:
> On 06/03/2019 04:25 PM, Tyrel Datwyler wrote:
>> On 06/03/2019 03:19 PM, Nathan Chancellor wrote:
>>> clang warns:
>>>
>>> drivers/scsi/ibmvscsi/ibmvscsi.c:2126:7: warning: variable 'rc' is used
>>> uninitialized whenever switch case is taken [-Wsometimes-uninitialized]
>>> case IBMVSCSI_HOST_ACTION_NONE:
>>> ^
>>> drivers/scsi/ibmvscsi/ibmvscsi.c:2151:6: note: uninitialized use occurs
>>> here
>>> if (rc) {
>>> ^~
>>>
>>> Initialize rc to zero in the case statements that clang mentions so that
>>> the atomic_set and dev_err statement don't trigger for them.
>>>
>>> Fixes: 035a3c4046b5 ("scsi: ibmvscsi: redo driver work thread to use enum
>>> action states")
>>> Link: https://github.com/ClangBuiltLinux/linux/issues/502
>>> Suggested-by: Michael Ellerman
>>> Signed-off-by: Nathan Chancellor
>>
>> Acked-by: Tyrel Datwyler
>>
>
> On second thought NACK. See my response to Michael earlier in the thread.
>
> I think this is the better solution:
>
> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c
> b/drivers/scsi/ibmvscsi/ibmvscsi.c
> index 727c31dc11a0..c3cf05dd8733 100644
> --- a/drivers/scsi/ibmvscsi/ibmvscsi.c
> +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
> @@ -2123,8 +2123,8 @@ static void ibmvscsi_do_work(struct ibmvscsi_host_data
> *hostdata)
>
> spin_lock_irqsave(hostdata->host->host_lock, flags);
> switch (hostdata->action) {
> - case IBMVSCSI_HOST_ACTION_NONE:
> case IBMVSCSI_HOST_ACTION_UNBLOCK:
> + rc = 0;
> break;
> case IBMVSCSI_HOST_ACTION_RESET:
> spin_unlock_irqrestore(hostdata->host->host_lock, flags);
> @@ -2142,8 +2142,9 @@ static void ibmvscsi_do_work(struct ibmvscsi_host_data
> *hostdata)
> if (!rc)
> rc = ibmvscsi_send_crq(hostdata,
> 0xC001LL, 0);
> break;
> + case IBMVSCSI_HOST_ACTION_NONE:
> default:
> - break;
Need a spin_unlock_irqrestore() here before the return.
-Tyrel
> + return;
> }
>
> hostdata->action = IBMVSCSI_HOST_ACTION_NONE;
>
Re: [PATCH v2] scsi: ibmvscsi: Don't use rc uninitialized in ibmvscsi_do_work
On 06/03/2019 04:25 PM, Tyrel Datwyler wrote:
> On 06/03/2019 03:19 PM, Nathan Chancellor wrote:
>> clang warns:
>>
>> drivers/scsi/ibmvscsi/ibmvscsi.c:2126:7: warning: variable 'rc' is used
>> uninitialized whenever switch case is taken [-Wsometimes-uninitialized]
>> case IBMVSCSI_HOST_ACTION_NONE:
>> ^
>> drivers/scsi/ibmvscsi/ibmvscsi.c:2151:6: note: uninitialized use occurs
>> here
>> if (rc) {
>> ^~
>>
>> Initialize rc to zero in the case statements that clang mentions so that
>> the atomic_set and dev_err statement don't trigger for them.
>>
>> Fixes: 035a3c4046b5 ("scsi: ibmvscsi: redo driver work thread to use enum
>> action states")
>> Link: https://github.com/ClangBuiltLinux/linux/issues/502
>> Suggested-by: Michael Ellerman
>> Signed-off-by: Nathan Chancellor
>
> Acked-by: Tyrel Datwyler
>
On second thought NACK. See my response to Michael earlier in the thread.
I think this is the better solution:
diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
index 727c31dc11a0..c3cf05dd8733 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.c
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
@@ -2123,8 +2123,8 @@ static void ibmvscsi_do_work(struct ibmvscsi_host_data
*hostdata)
spin_lock_irqsave(hostdata->host->host_lock, flags);
switch (hostdata->action) {
- case IBMVSCSI_HOST_ACTION_NONE:
case IBMVSCSI_HOST_ACTION_UNBLOCK:
+ rc = 0;
break;
case IBMVSCSI_HOST_ACTION_RESET:
spin_unlock_irqrestore(hostdata->host->host_lock, flags);
@@ -2142,8 +2142,9 @@ static void ibmvscsi_do_work(struct ibmvscsi_host_data
*hostdata)
if (!rc)
rc = ibmvscsi_send_crq(hostdata, 0xC001LL,
0);
break;
+ case IBMVSCSI_HOST_ACTION_NONE:
default:
- break;
+ return;
}
hostdata->action = IBMVSCSI_HOST_ACTION_NONE;
Re: [PATCH] scsi: ibmvscsi: Don't use rc uninitialized in ibmvscsi_do_work
On 06/02/2019 03:15 AM, Michael Ellerman wrote:
> Hi Nathan,
>
> Nathan Chancellor writes:
>> clang warns:
>>
>> drivers/scsi/ibmvscsi/ibmvscsi.c:2126:7: warning: variable 'rc' is used
>> uninitialized whenever switch case is taken [-Wsometimes-uninitialized]
>> case IBMVSCSI_HOST_ACTION_NONE:
>> ^
>> drivers/scsi/ibmvscsi/ibmvscsi.c:2151:6: note: uninitialized use occurs
>> here
>> if (rc) {
>> ^~
>>
>> Initialize rc to zero so that the atomic_set and dev_err statement don't
>> trigger for the cases that just break.
>>
>> Fixes: 035a3c4046b5 ("scsi: ibmvscsi: redo driver work thread to use enum
>> action states")
>> Link: https://github.com/ClangBuiltLinux/linux/issues/502
>> Signed-off-by: Nathan Chancellor
>> ---
>> drivers/scsi/ibmvscsi/ibmvscsi.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c
>> b/drivers/scsi/ibmvscsi/ibmvscsi.c
>> index 727c31dc11a0..6714d8043e62 100644
>> --- a/drivers/scsi/ibmvscsi/ibmvscsi.c
>> +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
>> @@ -2118,7 +2118,7 @@ static unsigned long ibmvscsi_get_desired_dma(struct
>> vio_dev *vdev)
>> static void ibmvscsi_do_work(struct ibmvscsi_host_data *hostdata)
>> {
>> unsigned long flags;
>> -int rc;
>> +int rc = 0;
>> char *action = "reset";
>>
>> spin_lock_irqsave(hostdata->host->host_lock, flags);
>
> It's always preferable IMHO to keep any initialisation as localised as
> possible, so that the compiler can continue to warn about uninitialised
> usages elsewhere. In this case that would mean doing the rc = 0 in the
> switch, something like:
>
> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c
> b/drivers/scsi/ibmvscsi/ibmvscsi.c
> index 727c31dc11a0..7ee5755cf636 100644
> --- a/drivers/scsi/ibmvscsi/ibmvscsi.c
> +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
> @@ -2123,9 +2123,6 @@ static void ibmvscsi_do_work(struct ibmvscsi_host_data
> *hostdata)
>
> spin_lock_irqsave(hostdata->host->host_lock, flags);
> switch (hostdata->action) {
> - case IBMVSCSI_HOST_ACTION_NONE:
> - case IBMVSCSI_HOST_ACTION_UNBLOCK:
> - break;
> case IBMVSCSI_HOST_ACTION_RESET:
> spin_unlock_irqrestore(hostdata->host->host_lock, flags);
> rc = ibmvscsi_reset_crq_queue(&hostdata->queue, hostdata);
> @@ -2142,7 +2139,10 @@ static void ibmvscsi_do_work(struct ibmvscsi_host_data
> *hostdata)
> if (!rc)
> rc = ibmvscsi_send_crq(hostdata,
> 0xC001LL, 0);
> break;
> + case IBMVSCSI_HOST_ACTION_NONE:
> + case IBMVSCSI_HOST_ACTION_UNBLOCK:
> default:
> + rc = 0;
> break;
> }
>
>
> But then that makes me wonder if that's actually correct?
>
> If we get an action that we don't recognise should we just throw it away
> like that? (by doing hostdata->action = IBMVSCSI_HOST_ACTION_NONE). Tyrel?
On initial pass I was ok with this, but after thinking on it I think it is more
subtle.
The right approach is to set rc = 0 for HOST_ACTION_UNBLOCK as we want to fall
through. For HOST_ACTION_NONE and default we need to return directly from the
function.
-Tyrel
>
> cheers
>
Re: [PATCH v2] scsi: ibmvscsi: Don't use rc uninitialized in ibmvscsi_do_work
On 06/03/2019 03:19 PM, Nathan Chancellor wrote:
> clang warns:
>
> drivers/scsi/ibmvscsi/ibmvscsi.c:2126:7: warning: variable 'rc' is used
> uninitialized whenever switch case is taken [-Wsometimes-uninitialized]
> case IBMVSCSI_HOST_ACTION_NONE:
> ^
> drivers/scsi/ibmvscsi/ibmvscsi.c:2151:6: note: uninitialized use occurs
> here
> if (rc) {
> ^~
>
> Initialize rc to zero in the case statements that clang mentions so that
> the atomic_set and dev_err statement don't trigger for them.
>
> Fixes: 035a3c4046b5 ("scsi: ibmvscsi: redo driver work thread to use enum
> action states")
> Link: https://github.com/ClangBuiltLinux/linux/issues/502
> Suggested-by: Michael Ellerman
> Signed-off-by: Nathan Chancellor
Acked-by: Tyrel Datwyler
Re: [PATCH 1/3] powerpc/pseries: Simplify cpu readd to use drc_index
On 05/20/2019 08:01 AM, Nathan Lynch wrote: > Tyrel Datwyler writes: > >> On 05/16/2019 12:17 PM, Nathan Lynch wrote: >>> Tyrel Datwyler writes: >>>> The current dlpar_cpu_readd() takes in a cpu_id and uses that to look up >>>> the cpus device_node so that we can get at the ibm,my-drc-index >>>> property. The only user of cpu readd is an OF notifier call back. This >>>> call back already has a reference to the device_node and therefore can >>>> retrieve the drc_index from the device_node. >>> >>> dlpar_cpu_readd is a hack to try to change the CPU-node relationship at >>> runtime without destabilizing the system. It doesn't accomplish that and >>> it should just be removed (and I'm working on that). >>> >> >> I will politely disagree. We've done exactly this from userspace for >> years. My experience still suggests that memory affinity is the >> problem area, and that the work to push this all into the kernel >> originally was poorly tested. > > Kernel implementation details aside, how do you change the cpu-node > relationship at runtime without breaking NUMA-aware applications? Is > this not a fundamental issue to address before adding code like this? > If that is the concern then hotplug in general already breaks them. Take for example the removal of a faulty processor and then adding a new processor back. It is quite possible that the new processor is in a different NUMA node. Keep in mind that in this scenario the new processor and threads gets the same logical cpu ids as the faulty processor we just removed. Now we have to ask the question who is right and who is wrong. In this case the kernel data structures reflect the correct NUMA topology. However, did the NUMA aware application or libnuma make an assumption that specific sets of logical cpu ids are always in the same NUMA node? -Tyrel
Re: [PATCH 1/3] powerpc/pseries: Simplify cpu readd to use drc_index
On 05/16/2019 12:17 PM, Nathan Lynch wrote: > Tyrel Datwyler writes: >> The current dlpar_cpu_readd() takes in a cpu_id and uses that to look up >> the cpus device_node so that we can get at the ibm,my-drc-index >> property. The only user of cpu readd is an OF notifier call back. This >> call back already has a reference to the device_node and therefore can >> retrieve the drc_index from the device_node. > > dlpar_cpu_readd is a hack to try to change the CPU-node relationship at > runtime without destabilizing the system. It doesn't accomplish that and > it should just be removed (and I'm working on that). > I will politely disagree. We've done exactly this from userspace for years. My experience still suggests that memory affinity is the problem area, and that the work to push this all into the kernel originally was poorly tested. -Tyrel
[PATCH 2/3] powerpc/pseries: Disable PRRN memory device tree trigger
Memory affintiy updates as currently implemented have proved unstable.
This patch comments out the PRRN hook for the time being while we
investigate how to either stablize the current implementation or find a
better approach.
Signed-off-by: Tyrel Datwyler
---
arch/powerpc/platforms/pseries/mobility.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/mobility.c
b/arch/powerpc/platforms/pseries/mobility.c
index 88925f8ca8a0..660a2dbc43d7 100644
--- a/arch/powerpc/platforms/pseries/mobility.c
+++ b/arch/powerpc/platforms/pseries/mobility.c
@@ -242,13 +242,15 @@ static int add_dt_node(__be32 parent_phandle, __be32
drc_index)
static void prrn_update_node(__be32 phandle)
{
+ /* PRRN Memory Updates have proved unstable. Disable for the time being.
+*
struct pseries_hp_errorlog hp_elog;
struct device_node *dn;
- /*
+*
* If a node is found from a the given phandle, the phandle does not
* represent the drc index of an LMB and we can ignore.
-*/
+*
dn = of_find_node_by_phandle(be32_to_cpu(phandle));
if (dn) {
of_node_put(dn);
@@ -261,6 +263,7 @@ static void prrn_update_node(__be32 phandle)
hp_elog._drc_u.drc_index = phandle;
handle_dlpar_errorlog(&hp_elog);
+ */
}
int pseries_devicetree_update(s32 scope)
--
2.18.1
[PATCH 3/3] powerpc/pseries: Don't update cpu topology after PRRN event
When we receive a PRRN event through the event-scan interface we call pseries_devicetree_update() to update the affinty properties in our device tree via RTAS. Following this our implemenation attempts to both frob the existing kernel cpu numa affinities of the live system with the new device tree properties while also performing a full cpu hotplug readd of the affected cpus in resposne to the a OF property notifier triggerd by the device tree update. This patch does away with the topology update call to frob the associativity since the DLPAR readd will put the cpu in the proper numa node when its added back. Signed-off-by: Tyrel Datwyler --- arch/powerpc/kernel/rtasd.c | 1 - 1 file changed, 1 deletion(-) diff --git a/arch/powerpc/kernel/rtasd.c b/arch/powerpc/kernel/rtasd.c index 8a1746d755c9..d3aa3a056d8e 100644 --- a/arch/powerpc/kernel/rtasd.c +++ b/arch/powerpc/kernel/rtasd.c @@ -285,7 +285,6 @@ static void handle_prrn_event(s32 scope) * the RTAS event. */ pseries_devicetree_update(-scope); - numa_update_cpu_topology(false); } static void handle_rtas_event(const struct rtas_error_log *log) -- 2.18.1
[PATCH 1/3] powerpc/pseries: Simplify cpu readd to use drc_index
The current dlpar_cpu_readd() takes in a cpu_id and uses that to look up
the cpus device_node so that we can get at the ibm,my-drc-index
property. The only user of cpu readd is an OF notifier call back. This
call back already has a reference to the device_node and therefore can
retrieve the drc_index from the device_node.
This patch simplifies dlpar_cpu_readd() to take a drc_index directly and
does away with an uneccsary device_node lookup.
Signed-off-by: Tyrel Datwyler
---
arch/powerpc/include/asm/topology.h | 2 +-
arch/powerpc/mm/numa.c | 6 +++---
arch/powerpc/platforms/pseries/hotplug-cpu.c | 10 +-
3 files changed, 5 insertions(+), 13 deletions(-)
diff --git a/arch/powerpc/include/asm/topology.h
b/arch/powerpc/include/asm/topology.h
index f85e2b01c3df..c906d9ec9013 100644
--- a/arch/powerpc/include/asm/topology.h
+++ b/arch/powerpc/include/asm/topology.h
@@ -133,7 +133,7 @@ static inline void shared_proc_topology_init(void) {}
#define topology_core_cpumask(cpu) (per_cpu(cpu_core_map, cpu))
#define topology_core_id(cpu) (cpu_to_core_id(cpu))
-int dlpar_cpu_readd(int cpu);
+int dlpar_cpu_readd(u32 drc_index);
#endif
#endif
diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
index 57e64273cb33..40c0b6da12c2 100644
--- a/arch/powerpc/mm/numa.c
+++ b/arch/powerpc/mm/numa.c
@@ -1479,9 +1479,9 @@ static int dt_update_callback(struct notifier_block *nb,
case OF_RECONFIG_UPDATE_PROPERTY:
if (of_node_is_type(update->dn, "cpu") &&
!of_prop_cmp(update->prop->name, "ibm,associativity")) {
- u32 core_id;
- of_property_read_u32(update->dn, "reg", &core_id);
- rc = dlpar_cpu_readd(core_id);
+ u32 drc_index;
+ of_property_read_u32(update->dn, "ibm,my-drc-index",
&drc_index);
+ rc = dlpar_cpu_readd(drc_index);
rc = NOTIFY_OK;
}
break;
diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c
b/arch/powerpc/platforms/pseries/hotplug-cpu.c
index 97feb6e79f1a..2dfa9416ce54 100644
--- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
+++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
@@ -802,18 +802,10 @@ static int dlpar_cpu_add_by_count(u32 cpus_to_add)
return rc;
}
-int dlpar_cpu_readd(int cpu)
+int dlpar_cpu_readd(u32 drc_index)
{
- struct device_node *dn;
- struct device *dev;
- u32 drc_index;
int rc;
- dev = get_cpu_device(cpu);
- dn = dev->of_node;
-
- rc = of_property_read_u32(dn, "ibm,my-drc-index", &drc_index);
-
rc = dlpar_cpu_remove_by_index(drc_index);
if (!rc)
rc = dlpar_cpu_add(drc_index);
--
2.18.1
Re: [RFC PATCH 1/3] powerpc/mm: Handle page table allocation failures
On 05/14/2019 07:50 AM, Aneesh Kumar K.V wrote:
> This fixes the below crash that arises due to not handling page table
> allocation
> failures while allocating hugetlb page table.
Was there supposed to be a oops stack trace attached here in the commit log?
-Tyrel
>
> Fixes: e2b3d202d1db ("powerpc: Switch 16GB and 16MB explicit hugepages to a
> different page table format")
> Signed-off-by: Aneesh Kumar K.V
> ---
> arch/powerpc/mm/hugetlbpage.c | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/arch/powerpc/mm/hugetlbpage.c b/arch/powerpc/mm/hugetlbpage.c
> index c5c9ff2d7afc..ae9d71da5219 100644
> --- a/arch/powerpc/mm/hugetlbpage.c
> +++ b/arch/powerpc/mm/hugetlbpage.c
> @@ -130,6 +130,8 @@ pte_t *huge_pte_alloc(struct mm_struct *mm, unsigned long
> addr, unsigned long sz
> } else {
> pdshift = PUD_SHIFT;
> pu = pud_alloc(mm, pg, addr);
> + if (!pu)
> + return NULL;
> if (pshift == PUD_SHIFT)
> return (pte_t *)pu;
> else if (pshift > PMD_SHIFT) {
> @@ -138,6 +140,8 @@ pte_t *huge_pte_alloc(struct mm_struct *mm, unsigned long
> addr, unsigned long sz
> } else {
> pdshift = PMD_SHIFT;
> pm = pmd_alloc(mm, pu, addr);
> + if (!pm)
> + return NULL;
> if (pshift == PMD_SHIFT)
> /* 16MB hugepage */
> return (pte_t *)pm;
> @@ -154,12 +158,16 @@ pte_t *huge_pte_alloc(struct mm_struct *mm, unsigned
> long addr, unsigned long sz
> } else {
> pdshift = PUD_SHIFT;
> pu = pud_alloc(mm, pg, addr);
> + if (!pu)
> + return NULL;
> if (pshift >= PUD_SHIFT) {
> ptl = pud_lockptr(mm, pu);
> hpdp = (hugepd_t *)pu;
> } else {
> pdshift = PMD_SHIFT;
> pm = pmd_alloc(mm, pu, addr);
> + if (!pm)
> + return NULL;
> ptl = pmd_lockptr(mm, pm);
> hpdp = (hugepd_t *)pm;
> }
>
[PATCH v2 2/3] ibmvscsi: redo driver work thread to use enum action states
From: Tyrel Datwyler
The current implemenation relies on two flags in the drivers private host
structure to signal the need for a host reset or to reenable the CRQ after a
LPAR migration. This patch does away with those flags and introduces a single
action flag and defined enums for the supported kthread work actions. Lastly,
the if/else logic is replaced with a switch statement.
Signed-off-by: Tyrel Datwyler
---
Changes in v2:
release/grab host_lock around reset/reenable calls
drivers/scsi/ibmvscsi/ibmvscsi.c | 61 ++--
drivers/scsi/ibmvscsi/ibmvscsi.h | 9 +++--
2 files changed, 49 insertions(+), 21 deletions(-)
diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
index 65fc8ca962c5..8df82c58e7b9 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.c
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
@@ -828,7 +828,7 @@ static void ibmvscsi_reset_host(struct ibmvscsi_host_data
*hostdata)
atomic_set(&hostdata->request_limit, 0);
purge_requests(hostdata, DID_ERROR);
- hostdata->reset_crq = 1;
+ hostdata->action = IBMVSCSI_HOST_ACTION_RESET;
wake_up(&hostdata->work_wait_q);
}
@@ -1797,7 +1797,7 @@ static void ibmvscsi_handle_crq(struct viosrp_crq *crq,
/* We need to re-setup the interpartition connection */
dev_info(hostdata->dev, "Re-enabling adapter!\n");
hostdata->client_migrated = 1;
- hostdata->reenable_crq = 1;
+ hostdata->action = IBMVSCSI_HOST_ACTION_REENABLE;
purge_requests(hostdata, DID_REQUEUE);
wake_up(&hostdata->work_wait_q);
} else {
@@ -2116,48 +2116,71 @@ static unsigned long ibmvscsi_get_desired_dma(struct
vio_dev *vdev)
static void ibmvscsi_do_work(struct ibmvscsi_host_data *hostdata)
{
+ unsigned long flags;
int rc;
char *action = "reset";
- if (hostdata->reset_crq) {
- smp_rmb();
- hostdata->reset_crq = 0;
-
+ spin_lock_irqsave(hostdata->host->host_lock, flags);
+ switch (hostdata->action) {
+ case IBMVSCSI_HOST_ACTION_NONE:
+ break;
+ case IBMVSCSI_HOST_ACTION_RESET:
+ spin_unlock_irqrestore(hostdata->host->host_lock, flags);
rc = ibmvscsi_reset_crq_queue(&hostdata->queue, hostdata);
+ spin_lock_irqsave(hostdata->host->host_lock, flags);
if (!rc)
rc = ibmvscsi_send_crq(hostdata, 0xC001LL,
0);
vio_enable_interrupts(to_vio_dev(hostdata->dev));
- } else if (hostdata->reenable_crq) {
- smp_rmb();
+ break;
+ case IBMVSCSI_HOST_ACTION_REENABLE:
action = "enable";
+ spin_unlock_irqrestore(hostdata->host->host_lock, flags);
rc = ibmvscsi_reenable_crq_queue(&hostdata->queue, hostdata);
- hostdata->reenable_crq = 0;
+ spin_lock_irqsave(hostdata->host->host_lock, flags);
if (!rc)
rc = ibmvscsi_send_crq(hostdata, 0xC001LL,
0);
- } else
- return;
+ break;
+ default:
+ break;
+ }
+
+ hostdata->action = IBMVSCSI_HOST_ACTION_NONE;
if (rc) {
atomic_set(&hostdata->request_limit, -1);
dev_err(hostdata->dev, "error after %s\n", action);
}
+ spin_unlock_irqrestore(hostdata->host->host_lock, flags);
scsi_unblock_requests(hostdata->host);
}
-static int ibmvscsi_work_to_do(struct ibmvscsi_host_data *hostdata)
+static int __ibmvscsi_work_to_do(struct ibmvscsi_host_data *hostdata)
{
if (kthread_should_stop())
return 1;
- else if (hostdata->reset_crq) {
- smp_rmb();
- return 1;
- } else if (hostdata->reenable_crq) {
- smp_rmb();
- return 1;
+ switch (hostdata->action) {
+ case IBMVSCSI_HOST_ACTION_NONE:
+ return 0;
+ case IBMVSCSI_HOST_ACTION_RESET:
+ case IBMVSCSI_HOST_ACTION_REENABLE:
+ default:
+ break;
}
- return 0;
+ return 1;
+}
+
+static int ibmvscsi_work_to_do(struct ibmvscsi_host_data *hostdata)
+{
+ unsigned long flags;
+ int rc;
+
+ spin_lock_irqsave(hostdata->host->host_lock, flags);
+ rc = __ibmvscsi_work_to_do(hostdata);
+ spin_unlock_irqrestore(hostdata->host->host_lock, flags);
+
+ return rc;
}
static int ibmvscsi_work(void *data)
diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.h b/drivers/scsi/ibmvscsi/ibmvscsi.h
index 3a787557
[PATCH v2 3/3] ibmvscsi: fix tripping of blk_mq_run_hw_queue WARN_ON
From: Tyrel Datwyler
After a successful SRP login response we call scsi_unblock_requests() to
kick any pending IO's. The callback to process this SRP response happens in
a tasklet and therefore is in softirq context. The result of such is
that when blk-mq is enabled it is no longer safe to call
scsi_unblock_requests() from this context. The result of duing so
triggers the following WARN_ON splat in dmesg after a host reset or CRQ
reenablement.
WARNING: CPU: 0 PID: 0 at block/blk-mq.c:1375 __blk_mq_run_hw_queue+0x120/0x180
Modules linked in:
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.0.0-rc8 #4
NIP [c09771e0] __blk_mq_run_hw_queue+0x120/0x180
LR [c0977484] __blk_mq_delay_run_hw_queue+0x244/0x250
Call Trace:
__blk_mq_delay_run_hw_queue+0x244/0x250
blk_mq_run_hw_queue+0x8c/0x1c0
blk_mq_run_hw_queues+0x60/0x90
scsi_run_queue+0x1e4/0x3b0
scsi_run_host_queues+0x48/0x80
login_rsp+0xb0/0x100
ibmvscsi_handle_crq+0x30c/0x3e0
ibmvscsi_task+0x54/0xe0
tasklet_action_common.isra.3+0xc4/0x1a0
__do_softirq+0x174/0x3f4
irq_exit+0xf0/0x120
__do_irq+0xb0/0x210
call_do_irq+0x14/0x24
do_IRQ+0x9c/0x130
hardware_interrupt_common+0x14c/0x150
This patch fixes the issue by introducing a new host action for
unblocking the scsi requests in our seperate work thread.
Signed-off-by: Tyrel Datwyler
---
Changes in v2:
no change
drivers/scsi/ibmvscsi/ibmvscsi.c | 5 -
drivers/scsi/ibmvscsi/ibmvscsi.h | 1 +
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
index 8df82c58e7b9..727c31dc11a0 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.c
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
@@ -1179,7 +1179,8 @@ static void login_rsp(struct srp_event_struct *evt_struct)
be32_to_cpu(evt_struct->xfer_iu->srp.login_rsp.req_lim_delta));
/* If we had any pending I/Os, kick them */
- scsi_unblock_requests(hostdata->host);
+ hostdata->action = IBMVSCSI_HOST_ACTION_UNBLOCK;
+ wake_up(&hostdata->work_wait_q);
}
/**
@@ -2123,6 +2124,7 @@ static void ibmvscsi_do_work(struct ibmvscsi_host_data
*hostdata)
spin_lock_irqsave(hostdata->host->host_lock, flags);
switch (hostdata->action) {
case IBMVSCSI_HOST_ACTION_NONE:
+ case IBMVSCSI_HOST_ACTION_UNBLOCK:
break;
case IBMVSCSI_HOST_ACTION_RESET:
spin_unlock_irqrestore(hostdata->host->host_lock, flags);
@@ -2164,6 +2166,7 @@ static int __ibmvscsi_work_to_do(struct
ibmvscsi_host_data *hostdata)
return 0;
case IBMVSCSI_HOST_ACTION_RESET:
case IBMVSCSI_HOST_ACTION_REENABLE:
+ case IBMVSCSI_HOST_ACTION_UNBLOCK:
default:
break;
}
diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.h b/drivers/scsi/ibmvscsi/ibmvscsi.h
index 04bcbc832dc9..d9bf502334ba 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.h
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.h
@@ -92,6 +92,7 @@ enum ibmvscsi_host_action {
IBMVSCSI_HOST_ACTION_NONE = 0,
IBMVSCSI_HOST_ACTION_RESET,
IBMVSCSI_HOST_ACTION_REENABLE,
+ IBMVSCSI_HOST_ACTION_UNBLOCK,
};
/* all driver data associated with a host adapter */
--
2.18.1
[PATCH v2 1/3] ibmvscsi: Wire up host_reset() in the drivers scsi_host_template
From: Tyrel Datwyler
Wire up the host_reset function in our driver_template to allow a user
requested adpater reset via the host_reset sysfs attribute.
Example:
echo "adapter" > /sys/class/scsi_host/host0/host_reset
Signed-off-by: Tyrel Datwyler
---
Changes in v2:
removed interrupt disabe/enable around reset
drivers/scsi/ibmvscsi/ibmvscsi.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
index 8cec5230fe31..65fc8ca962c5 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.c
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
@@ -2050,6 +2050,16 @@ static struct device_attribute ibmvscsi_host_config = {
.show = show_host_config,
};
+static int ibmvscsi_host_reset(struct Scsi_Host *shost, int reset_type)
+{
+ struct ibmvscsi_host_data *hostdata = shost_priv(shost);
+
+ dev_info(hostdata->dev, "Initiating adapter reset!\n");
+ ibmvscsi_reset_host(hostdata);
+
+ return 0;
+}
+
static struct device_attribute *ibmvscsi_attrs[] = {
&ibmvscsi_host_vhost_loc,
&ibmvscsi_host_vhost_name,
@@ -2076,6 +2086,7 @@ static struct scsi_host_template driver_template = {
.eh_host_reset_handler = ibmvscsi_eh_host_reset_handler,
.slave_configure = ibmvscsi_slave_configure,
.change_queue_depth = ibmvscsi_change_queue_depth,
+ .host_reset = ibmvscsi_host_reset,
.cmd_per_lun = IBMVSCSI_CMDS_PER_LUN_DEFAULT,
.can_queue = IBMVSCSI_MAX_REQUESTS_DEFAULT,
.this_id = -1,
--
2.18.1
Re: [PATCH 1/3] ibmvscsi: Wire up host_reset() in the drivers scsi_host_template
On 05/02/2019 02:50 PM, Brian King wrote:
> On 5/1/19 7:47 PM, Tyrel Datwyler wrote:
>> From: Tyrel Datwyler
>>
>> Wire up the host_reset function in our driver_template to allow a user
>> requested adpater reset via the host_reset sysfs attribute.
>>
>> Example:
>>
>> echo "adapter" > /sys/class/scsi_host/host0/host_reset
>>
>> Signed-off-by: Tyrel Datwyler
>> ---
>> drivers/scsi/ibmvscsi/ibmvscsi.c | 13 +
>> 1 file changed, 13 insertions(+)
>>
>> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c
>> b/drivers/scsi/ibmvscsi/ibmvscsi.c
>> index 8cec5230fe31..1c37244f16a0 100644
>> --- a/drivers/scsi/ibmvscsi/ibmvscsi.c
>> +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
>> @@ -2050,6 +2050,18 @@ static struct device_attribute ibmvscsi_host_config =
>> {
>> .show = show_host_config,
>> };
>>
>> +static int ibmvscsi_host_reset(struct Scsi_Host *shost, int reset_type)
>> +{
>> +struct ibmvscsi_host_data *hostdata = shost_priv(shost);
>> +
>> +vio_disable_interrupts(to_vio_dev(hostdata->dev));
>> +dev_info(hostdata->dev, "Initiating adapter reset!\n");
>> +ibmvscsi_reset_host(hostdata);
>> +vio_enable_interrupts(to_vio_dev(hostdata->dev));
>
> Is it necessary to disable / enable interrupts around the call to
> ibmvscsi_reset_host?
> I don't know why we'd need to do that before calling the reset as we have
> other
> cases, like ibmvscsi_timeout where we don't bother doing this. Also, at the
> end
> of the reset we look to be already enabling interrupts.
Yeah, I think you are right. My initial line of thought was that we have
interrupts disabled in handle_crq when we do a reset, but yeah we clearly call
it in the case of a timeout with them enabled.
-Tyrel
>
> Thanks,
>
> Brian
>
Re: [PATCH 2/3] ibmvscsi: redo driver work thread to use enum action states
On 05/02/2019 02:43 PM, Brian King wrote:
> On 5/1/19 7:47 PM, Tyrel Datwyler wrote:
>> From: Tyrel Datwyler
>>
>> The current implemenation relies on two flags in the drivers private host
>> structure to signal the need for a host reset or to reenable the CRQ after a
>> LPAR migration. This patch does away with those flags and introduces a single
>> action flag and defined enums for the supported kthread work actions. Lastly,
>> the if/else logic is replaced with a switch statement.
>>
>> Signed-off-by: Tyrel Datwyler
>> ---
>> drivers/scsi/ibmvscsi/ibmvscsi.c | 57 +---
>> drivers/scsi/ibmvscsi/ibmvscsi.h | 9 +++--
>> 2 files changed, 45 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c
>> b/drivers/scsi/ibmvscsi/ibmvscsi.c
>> index 1c37244f16a0..683139e6c63f 100644
>> --- a/drivers/scsi/ibmvscsi/ibmvscsi.c
>> +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
>> @@ -828,7 +828,7 @@ static void ibmvscsi_reset_host(struct
>> ibmvscsi_host_data *hostdata)
>> atomic_set(&hostdata->request_limit, 0);
>>
>> purge_requests(hostdata, DID_ERROR);
>> -hostdata->reset_crq = 1;
>> +hostdata->action = IBMVSCSI_HOST_ACTION_RESET;
>> wake_up(&hostdata->work_wait_q);
>> }
>>
>> @@ -1797,7 +1797,7 @@ static void ibmvscsi_handle_crq(struct viosrp_crq *crq,
>> /* We need to re-setup the interpartition connection */
>> dev_info(hostdata->dev, "Re-enabling adapter!\n");
>> hostdata->client_migrated = 1;
>> -hostdata->reenable_crq = 1;
>> +hostdata->action = IBMVSCSI_HOST_ACTION_REENABLE;
>> purge_requests(hostdata, DID_REQUEUE);
>> wake_up(&hostdata->work_wait_q);
>> } else {
>> @@ -2118,26 +2118,32 @@ static unsigned long ibmvscsi_get_desired_dma(struct
>> vio_dev *vdev)
>>
>> static void ibmvscsi_do_work(struct ibmvscsi_host_data *hostdata)
>> {
>> +unsigned long flags;
>> int rc;
>> char *action = "reset";
>>
>> -if (hostdata->reset_crq) {
>> -smp_rmb();
>> -hostdata->reset_crq = 0;
>> -
>> +spin_lock_irqsave(hostdata->host->host_lock, flags);
>> +switch (hostdata->action) {
>> +case IBMVSCSI_HOST_ACTION_NONE:
>> +break;
>> +case IBMVSCSI_HOST_ACTION_RESET:
>> rc = ibmvscsi_reset_crq_queue(&hostdata->queue, hostdata);
>
> Looks like you are now calling ibmvscsi_reset_crq_queue with the host_lock
> held.
> However, ibmvscsi_reset_crq_queue can call msleep.
Good catch. I remember thinking that needed to run lockless, but clearly failed
to release and re-grab the lock around that call.
-Tyrel
>
> This had been implemented as separate reset_crq and reenable_crq fields
> so that it could run lockless. I'm not opposed to changing this to a single
> field in general, we just need to be careful where we are adding locking.
>
> Thanks,
>
> Brian
>
[PATCH 3/3] ibmvscsi: fix tripping of blk_mq_run_hw_queue WARN_ON
From: Tyrel Datwyler
After a successful SRP login response we call scsi_unblock_requests() to
kick any pending IO's. The callback to process this SRP response happens in
a tasklet and therefore is in softirq context. The result of such is
that when blk-mq is enabled it is no longer safe to call
scsi_unblock_requests() from this context. The result of duing so
triggers the following WARN_ON splat in dmesg after a host reset or CRQ
reenablement.
WARNING: CPU: 0 PID: 0 at block/blk-mq.c:1375 __blk_mq_run_hw_queue+0x120/0x180
Modules linked in:
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.0.0-rc8 #4
NIP [c09771e0] __blk_mq_run_hw_queue+0x120/0x180
LR [c0977484] __blk_mq_delay_run_hw_queue+0x244/0x250
Call Trace:
__blk_mq_delay_run_hw_queue+0x244/0x250
blk_mq_run_hw_queue+0x8c/0x1c0
blk_mq_run_hw_queues+0x60/0x90
scsi_run_queue+0x1e4/0x3b0
scsi_run_host_queues+0x48/0x80
login_rsp+0xb0/0x100
ibmvscsi_handle_crq+0x30c/0x3e0
ibmvscsi_task+0x54/0xe0
tasklet_action_common.isra.3+0xc4/0x1a0
__do_softirq+0x174/0x3f4
irq_exit+0xf0/0x120
__do_irq+0xb0/0x210
call_do_irq+0x14/0x24
do_IRQ+0x9c/0x130
hardware_interrupt_common+0x14c/0x150
This patch fixes the issue by introducing a new host action for
unblocking the scsi requests in our seperate work thread.
Signed-off-by: Tyrel Datwyler
---
drivers/scsi/ibmvscsi/ibmvscsi.c | 5 -
drivers/scsi/ibmvscsi/ibmvscsi.h | 1 +
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
index 683139e6c63f..c1d83eb5c5f7 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.c
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
@@ -1179,7 +1179,8 @@ static void login_rsp(struct srp_event_struct *evt_struct)
be32_to_cpu(evt_struct->xfer_iu->srp.login_rsp.req_lim_delta));
/* If we had any pending I/Os, kick them */
- scsi_unblock_requests(hostdata->host);
+ hostdata->action = IBMVSCSI_HOST_ACTION_UNBLOCK;
+ wake_up(&hostdata->work_wait_q);
}
/**
@@ -2125,6 +2126,7 @@ static void ibmvscsi_do_work(struct ibmvscsi_host_data
*hostdata)
spin_lock_irqsave(hostdata->host->host_lock, flags);
switch (hostdata->action) {
case IBMVSCSI_HOST_ACTION_NONE:
+ case IBMVSCSI_HOST_ACTION_UNBLOCK:
break;
case IBMVSCSI_HOST_ACTION_RESET:
rc = ibmvscsi_reset_crq_queue(&hostdata->queue, hostdata);
@@ -2162,6 +2164,7 @@ static int __ibmvscsi_work_to_do(struct
ibmvscsi_host_data *hostdata)
return 0;
case IBMVSCSI_HOST_ACTION_RESET:
case IBMVSCSI_HOST_ACTION_REENABLE:
+ case IBMVSCSI_HOST_ACTION_UNBLOCK:
default:
break;
}
diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.h b/drivers/scsi/ibmvscsi/ibmvscsi.h
index 04bcbc832dc9..d9bf502334ba 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.h
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.h
@@ -92,6 +92,7 @@ enum ibmvscsi_host_action {
IBMVSCSI_HOST_ACTION_NONE = 0,
IBMVSCSI_HOST_ACTION_RESET,
IBMVSCSI_HOST_ACTION_REENABLE,
+ IBMVSCSI_HOST_ACTION_UNBLOCK,
};
/* all driver data associated with a host adapter */
--
2.18.1
[PATCH 2/3] ibmvscsi: redo driver work thread to use enum action states
From: Tyrel Datwyler
The current implemenation relies on two flags in the drivers private host
structure to signal the need for a host reset or to reenable the CRQ after a
LPAR migration. This patch does away with those flags and introduces a single
action flag and defined enums for the supported kthread work actions. Lastly,
the if/else logic is replaced with a switch statement.
Signed-off-by: Tyrel Datwyler
---
drivers/scsi/ibmvscsi/ibmvscsi.c | 57 +---
drivers/scsi/ibmvscsi/ibmvscsi.h | 9 +++--
2 files changed, 45 insertions(+), 21 deletions(-)
diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
index 1c37244f16a0..683139e6c63f 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.c
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
@@ -828,7 +828,7 @@ static void ibmvscsi_reset_host(struct ibmvscsi_host_data
*hostdata)
atomic_set(&hostdata->request_limit, 0);
purge_requests(hostdata, DID_ERROR);
- hostdata->reset_crq = 1;
+ hostdata->action = IBMVSCSI_HOST_ACTION_RESET;
wake_up(&hostdata->work_wait_q);
}
@@ -1797,7 +1797,7 @@ static void ibmvscsi_handle_crq(struct viosrp_crq *crq,
/* We need to re-setup the interpartition connection */
dev_info(hostdata->dev, "Re-enabling adapter!\n");
hostdata->client_migrated = 1;
- hostdata->reenable_crq = 1;
+ hostdata->action = IBMVSCSI_HOST_ACTION_REENABLE;
purge_requests(hostdata, DID_REQUEUE);
wake_up(&hostdata->work_wait_q);
} else {
@@ -2118,26 +2118,32 @@ static unsigned long ibmvscsi_get_desired_dma(struct
vio_dev *vdev)
static void ibmvscsi_do_work(struct ibmvscsi_host_data *hostdata)
{
+ unsigned long flags;
int rc;
char *action = "reset";
- if (hostdata->reset_crq) {
- smp_rmb();
- hostdata->reset_crq = 0;
-
+ spin_lock_irqsave(hostdata->host->host_lock, flags);
+ switch (hostdata->action) {
+ case IBMVSCSI_HOST_ACTION_NONE:
+ break;
+ case IBMVSCSI_HOST_ACTION_RESET:
rc = ibmvscsi_reset_crq_queue(&hostdata->queue, hostdata);
if (!rc)
rc = ibmvscsi_send_crq(hostdata, 0xC001LL,
0);
vio_enable_interrupts(to_vio_dev(hostdata->dev));
- } else if (hostdata->reenable_crq) {
- smp_rmb();
+ break;
+ case IBMVSCSI_HOST_ACTION_REENABLE:
action = "enable";
rc = ibmvscsi_reenable_crq_queue(&hostdata->queue, hostdata);
- hostdata->reenable_crq = 0;
if (!rc)
rc = ibmvscsi_send_crq(hostdata, 0xC001LL,
0);
- } else
- return;
+ break;
+ default:
+ break;
+ }
+
+ hostdata->action = IBMVSCSI_HOST_ACTION_NONE;
+ spin_unlock_irqrestore(hostdata->host->host_lock, flags);
if (rc) {
atomic_set(&hostdata->request_limit, -1);
@@ -2147,19 +2153,32 @@ static void ibmvscsi_do_work(struct ibmvscsi_host_data
*hostdata)
scsi_unblock_requests(hostdata->host);
}
-static int ibmvscsi_work_to_do(struct ibmvscsi_host_data *hostdata)
+static int __ibmvscsi_work_to_do(struct ibmvscsi_host_data *hostdata)
{
if (kthread_should_stop())
return 1;
- else if (hostdata->reset_crq) {
- smp_rmb();
- return 1;
- } else if (hostdata->reenable_crq) {
- smp_rmb();
- return 1;
+ switch (hostdata->action) {
+ case IBMVSCSI_HOST_ACTION_NONE:
+ return 0;
+ case IBMVSCSI_HOST_ACTION_RESET:
+ case IBMVSCSI_HOST_ACTION_REENABLE:
+ default:
+ break;
}
- return 0;
+ return 1;
+}
+
+static int ibmvscsi_work_to_do(struct ibmvscsi_host_data *hostdata)
+{
+ unsigned long flags;
+ int rc;
+
+ spin_lock_irqsave(hostdata->host->host_lock, flags);
+ rc = __ibmvscsi_work_to_do(hostdata);
+ spin_unlock_irqrestore(hostdata->host->host_lock, flags);
+
+ return rc;
}
static int ibmvscsi_work(void *data)
diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.h b/drivers/scsi/ibmvscsi/ibmvscsi.h
index 3a7875575616..04bcbc832dc9 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.h
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.h
@@ -88,13 +88,18 @@ struct event_pool {
dma_addr_t iu_token;
};
+enum ibmvscsi_host_action {
+ IBMVSCSI_HOST_ACTION_NONE = 0,
+ IBMVSCSI_HOST_ACTION_RESET,
+ IBMVSCSI_HOST_ACTION_REENABLE,
+};
+
/* all driver data associated with a host adapter */
struct ibmvs
[PATCH 1/3] ibmvscsi: Wire up host_reset() in the drivers scsi_host_template
From: Tyrel Datwyler
Wire up the host_reset function in our driver_template to allow a user
requested adpater reset via the host_reset sysfs attribute.
Example:
echo "adapter" > /sys/class/scsi_host/host0/host_reset
Signed-off-by: Tyrel Datwyler
---
drivers/scsi/ibmvscsi/ibmvscsi.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
index 8cec5230fe31..1c37244f16a0 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.c
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
@@ -2050,6 +2050,18 @@ static struct device_attribute ibmvscsi_host_config = {
.show = show_host_config,
};
+static int ibmvscsi_host_reset(struct Scsi_Host *shost, int reset_type)
+{
+ struct ibmvscsi_host_data *hostdata = shost_priv(shost);
+
+ vio_disable_interrupts(to_vio_dev(hostdata->dev));
+ dev_info(hostdata->dev, "Initiating adapter reset!\n");
+ ibmvscsi_reset_host(hostdata);
+ vio_enable_interrupts(to_vio_dev(hostdata->dev));
+
+ return 0;
+}
+
static struct device_attribute *ibmvscsi_attrs[] = {
&ibmvscsi_host_vhost_loc,
&ibmvscsi_host_vhost_name,
@@ -2076,6 +2088,7 @@ static struct scsi_host_template driver_template = {
.eh_host_reset_handler = ibmvscsi_eh_host_reset_handler,
.slave_configure = ibmvscsi_slave_configure,
.change_queue_depth = ibmvscsi_change_queue_depth,
+ .host_reset = ibmvscsi_host_reset,
.cmd_per_lun = IBMVSCSI_CMDS_PER_LUN_DEFAULT,
.can_queue = IBMVSCSI_MAX_REQUESTS_DEFAULT,
.this_id = -1,
--
2.18.1
Re: [PATCH] powerpc: Fix kobject memleak
On 04/29/2019 06:09 PM, Tobin C. Harding wrote: > Currently error return from kobject_init_and_add() is not followed by a > call to kobject_put(). This means there is a memory leak. > > Add call to kobject_put() in error path of kobject_init_and_add(). > > Signed-off-by: Tobin C. Harding > --- Reviewed-by: Tyrel Datwyler
Re: [PATCH v2] powerpc/pseries: Use correct event modifier in rtas_parse_epow_errlog()
On 04/23/2019 07:17 PM, Yue Haibing wrote:
> From: YueHaibing
>
> rtas_parse_epow_errlog() should pass 'modifier' to
> handle_system_shutdown, because event modifier only use
> bottom 4 bits.
>
> Fixes: 55fc0c561742 ("powerpc/pseries: Parse and handle EPOW interrupts")
> Signed-off-by: YueHaibing
> ---
Reviewed-by: Tyrel Datwyler
> v2: fix compile issue by 'event_modifier'-->'modifier'
> ---
> arch/powerpc/platforms/pseries/ras.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/platforms/pseries/ras.c
> b/arch/powerpc/platforms/pseries/ras.c
> index c97d153..744604d 100644
> --- a/arch/powerpc/platforms/pseries/ras.c
> +++ b/arch/powerpc/platforms/pseries/ras.c
> @@ -285,7 +285,7 @@ static void rtas_parse_epow_errlog(struct rtas_error_log
> *log)
> break;
>
> case EPOW_SYSTEM_SHUTDOWN:
> - handle_system_shutdown(epow_log->event_modifier);
> + handle_system_shutdown(modifier);
> break;
>
> case EPOW_SYSTEM_HALT:
>
Re: [PATCH 0/8]
On 04/14/2019 08:41 PM, Sam Bobroff wrote:
> On Thu, Apr 11, 2019 at 05:55:33PM -0700, Tyrel Datwyler wrote:
>> On 03/19/2019 07:58 PM, Sam Bobroff wrote:
>>> Hi all,
>>>
>>> This patch set adds support for EEH recovery of hot plugged devices on
>>> pSeries
>>> machines. Specifically, devices discovered by PCI rescanning using
>>> /sys/bus/pci/rescan, which includes devices hotplugged by QEMU's device_add
>>> command. (pSeries doesn't currently use slot power control for hotplugging.)
>>
>> Slight nit that its not that pSeries doesn't support slot power control
>> hotplugging, its that QEMU pSeries guests don't support it. We most
>> definitely
>> use the slot power control for hotplugging in PowerVM pSeries Linux guests.
>> More
>
> Ah, I think I see what you mean: pSeries can (and does!) use slot power
> control for hotplugging, it's just that Linux doesn't. Right :-) I'll
> change it to "Linux on pSeries doesn't" for the next version.
Not quite. A pSeries Linux PowerVM LPAR does use the slot power hotplugging. It
is only the pSeries Linux qemu guest that doesn't. The way that hotplug is
initiated is different for PowerVM and qemu. On PowerVM the command is sent from
the HMC down the RSCT stack which calls drmgr whereas with qemu hotplug
generates an interrupt which logs an event that is picked up by rtas_errd which
then calls drmgr with the "-V" flag. That flag was a special case that we added
to drmgr to work around the slot power hotplugging with rpaphp not working
correctly with qemu guests.
>
>> specifically we had to work around short comings in the rpaphp driver when
>> dealing with QEMU. This being that while at initial glance the design implies
>> that it had multiple devices per PHB in mind, it didn't, and only actually
>> supported a single slot per PHB. Further, when we developed the QEMU pci
>> hotplug
>> feature we had to deal with only having a single PHB per QEMU guest and as a
>> result needed a way to plug multiple pci devices into a single PHB. Hence,
>> came
>> the pci rescan work around in drmgr.
>>
>> Mike Roth and I have had discussions over the years to get the slot power
>> control hotplugging working properly with QEMU, and while I did get the RPA
>> hotplug driver fixed to register all available slots associated with a PHB,
>> EEH
>> remained an issue. So, I'm very happy to see this patchset get that working
>> with
>> the rescan work around.
>>
>>>
>>> As a side effect this also provides EEH support for devices removed by
>>> /sys/bus/pci/devices/*/remove and re-discovered by writing to
>>> /sys/bus/pci/rescan,
>>> on all platforms.
>>
>> +1, this seems like icing on the cake. ;)
>
> Yes :-)
>
> Although maybe I should mention that we can't really benefit from this
> on PowerNV *yet* because there seem to be some other problems with
> removing and re-scanning devices: in my tests devices are often unusable
> after being rediscovered.
>
> (I'm hoping to take a look at that soon.)
Interesting.
>
>>>
>>> The approach I've taken is to use the fact that the existing
>>> pcibios_bus_add_device() platform hooks (which are used to set up EEH on
>>> Virtual Function devices (VFs)) are actually called for all devices, so I've
>>> widened their scope and made other adjustments necessary to allow them to
>>> work
>>> for hotplugged and boot-time devices as well.
>>>
>>> Because some of the changes are in generic PowerPC code, it's
>>> possible that I've disturbed something for another PowerPC platform. I've
>>> tried
>>> to minimize this by leaving that code alone as much as possible and so there
>>> are a few cases where eeh_add_device_{early,late}() or
>>> eeh_add_sysfs_files() is
>>> called more than once. I think these can be looked at later, as duplicate
>>> calls
>>> are not harmful.
>>>
>>> The patch "Convert PNV_PHB_FLAG_EEH" isn't strictly necessary and I'm not
>>> sure
>>> if it's better to keep it, because it simplifies the code or drop it,
>>> because
>>> we may need a separate flag per PHB later on. Thoughts anyone?
>>>
>>> The first patch is a rework of the pcibios_init reordering patch I posted
>>> earlier, which I've included here because it's necessary for this set.
>>>
>>> I have done some testing for PowerNV on Power9 using a modifi
Re: [PATCH 0/8]
On 03/19/2019 07:58 PM, Sam Bobroff wrote:
> Hi all,
>
> This patch set adds support for EEH recovery of hot plugged devices on pSeries
> machines. Specifically, devices discovered by PCI rescanning using
> /sys/bus/pci/rescan, which includes devices hotplugged by QEMU's device_add
> command. (pSeries doesn't currently use slot power control for hotplugging.)
Slight nit that its not that pSeries doesn't support slot power control
hotplugging, its that QEMU pSeries guests don't support it. We most definitely
use the slot power control for hotplugging in PowerVM pSeries Linux guests. More
specifically we had to work around short comings in the rpaphp driver when
dealing with QEMU. This being that while at initial glance the design implies
that it had multiple devices per PHB in mind, it didn't, and only actually
supported a single slot per PHB. Further, when we developed the QEMU pci hotplug
feature we had to deal with only having a single PHB per QEMU guest and as a
result needed a way to plug multiple pci devices into a single PHB. Hence, came
the pci rescan work around in drmgr.
Mike Roth and I have had discussions over the years to get the slot power
control hotplugging working properly with QEMU, and while I did get the RPA
hotplug driver fixed to register all available slots associated with a PHB, EEH
remained an issue. So, I'm very happy to see this patchset get that working with
the rescan work around.
>
> As a side effect this also provides EEH support for devices removed by
> /sys/bus/pci/devices/*/remove and re-discovered by writing to
> /sys/bus/pci/rescan,
> on all platforms.
+1, this seems like icing on the cake. ;)
>
> The approach I've taken is to use the fact that the existing
> pcibios_bus_add_device() platform hooks (which are used to set up EEH on
> Virtual Function devices (VFs)) are actually called for all devices, so I've
> widened their scope and made other adjustments necessary to allow them to work
> for hotplugged and boot-time devices as well.
>
> Because some of the changes are in generic PowerPC code, it's
> possible that I've disturbed something for another PowerPC platform. I've
> tried
> to minimize this by leaving that code alone as much as possible and so there
> are a few cases where eeh_add_device_{early,late}() or eeh_add_sysfs_files()
> is
> called more than once. I think these can be looked at later, as duplicate
> calls
> are not harmful.
>
> The patch "Convert PNV_PHB_FLAG_EEH" isn't strictly necessary and I'm not sure
> if it's better to keep it, because it simplifies the code or drop it, because
> we may need a separate flag per PHB later on. Thoughts anyone?
>
> The first patch is a rework of the pcibios_init reordering patch I posted
> earlier, which I've included here because it's necessary for this set.
>
> I have done some testing for PowerNV on Power9 using a modified pnv_php module
> and some testing on pSeries with slot power control using a modified rpaphp
> module, and the EEH-related parts seem to work.
I'm interested in what modifications with rpaphp. Its unclear if you are saying
rpaphp modified so that slot power hotplug works with a QEMU pSeries guest? If
thats the case it would be optimal to get that upstream and remove the work
rescan workaround for guests that don't need it.
-Tyrel
>
> Cheers,
> Sam.
>
> Sam Bobroff (8):
> powerpc/64: Adjust order in pcibios_init()
> powerpc/eeh: Clear stale EEH_DEV_NO_HANDLER flag
> powerpc/eeh: Convert PNV_PHB_FLAG_EEH to global flag
> powerpc/eeh: Improve debug messages around device addition
> powerpc/eeh: Add eeh_show_enabled()
> powerpc/eeh: Initialize EEH address cache earlier
> powerpc/eeh: EEH for pSeries hot plug
> powerpc/eeh: Remove eeh_probe_devices() and eeh_addr_cache_build()
>
> arch/powerpc/include/asm/eeh.h | 19 +++--
> arch/powerpc/kernel/eeh.c| 33 -
> arch/powerpc/kernel/eeh_cache.c | 29 +---
> arch/powerpc/kernel/eeh_driver.c | 4 ++
> arch/powerpc/kernel/of_platform.c| 3 +-
> arch/powerpc/kernel/pci-common.c | 4 --
> arch/powerpc/kernel/pci_32.c | 4 ++
> arch/powerpc/kernel/pci_64.c | 12 +++-
> arch/powerpc/platforms/powernv/eeh-powernv.c | 41 +--
> arch/powerpc/platforms/powernv/pci.c | 7 +-
> arch/powerpc/platforms/powernv/pci.h | 2 -
> arch/powerpc/platforms/pseries/eeh_pseries.c | 75 +++-
> arch/powerpc/platforms/pseries/pci.c | 7 +-
> 13 files changed, 122 insertions(+), 118 deletions(-)
>
[PATCH 2/2] pci: rpaphp: get/put device node reference during slot alloc/dealloc
When allocating the slot structure we store a pointer to the associated
device_node. We really should be incrementing the reference count, so
add an of_node_get() during slot alloc and an of_node_put() during slot
dealloc.
Signed-off-by: Tyrel Datwyler
---
drivers/pci/hotplug/rpaphp_slot.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/hotplug/rpaphp_slot.c
b/drivers/pci/hotplug/rpaphp_slot.c
index 5282aa3e33c5..93b4a945c55d 100644
--- a/drivers/pci/hotplug/rpaphp_slot.c
+++ b/drivers/pci/hotplug/rpaphp_slot.c
@@ -21,6 +21,7 @@
/* free up the memory used by a slot */
void dealloc_slot_struct(struct slot *slot)
{
+ of_node_put(slot->dn);
kfree(slot->name);
kfree(slot);
}
@@ -36,7 +37,7 @@ struct slot *alloc_slot_struct(struct device_node *dn,
slot->name = kstrdup(drc_name, GFP_KERNEL);
if (!slot->name)
goto error_slot;
- slot->dn = dn;
+ slot->dn = of_node_get(dn);
slot->index = drc_index;
slot->power_domain = power_domain;
slot->hotplug_slot.ops = &rpaphp_hotplug_slot_ops;
--
2.12.3
[PATCH 1/2] pci: rpadlpar: fix leaked device_node references in add/remove paths
The find_dlpar_node() helper returns a device node with its reference
incremented. Both the add and remove paths use this helper for find the
appropriate node, but fail to release the reference when done.
Annotate the find_dlpar_node() helper with a comment about the incremented
reference count, and call of_node_put() on the obtained device_node in the
add and remove paths. Also, fixup a reference leak in the find_vio_slot()
helper where we fail to call of_node_put() on the vdevice node after we
iterate over its children.
Signed-off-by: Tyrel Datwyler
---
drivers/pci/hotplug/rpadlpar_core.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/pci/hotplug/rpadlpar_core.c
b/drivers/pci/hotplug/rpadlpar_core.c
index e2356a9c7088..182f9e3443ee 100644
--- a/drivers/pci/hotplug/rpadlpar_core.c
+++ b/drivers/pci/hotplug/rpadlpar_core.c
@@ -51,6 +51,7 @@ static struct device_node *find_vio_slot_node(char *drc_name)
if (rc == 0)
break;
}
+ of_node_put(parent);
return dn;
}
@@ -71,6 +72,7 @@ static struct device_node *find_php_slot_pci_node(char
*drc_name,
return np;
}
+/* Returns a device_node with its reference count incremented */
static struct device_node *find_dlpar_node(char *drc_name, int *node_type)
{
struct device_node *dn;
@@ -306,6 +308,7 @@ int dlpar_add_slot(char *drc_name)
rc = dlpar_add_phb(drc_name, dn);
break;
}
+ of_node_put(dn);
printk(KERN_INFO "%s: slot %s added\n", DLPAR_MODULE_NAME, drc_name);
exit:
@@ -439,6 +442,7 @@ int dlpar_remove_slot(char *drc_name)
rc = dlpar_remove_pci_slot(drc_name, dn);
break;
}
+ of_node_put(dn);
vm_unmap_aliases();
printk(KERN_INFO "%s: slot %s removed\n", DLPAR_MODULE_NAME, drc_name);
--
2.12.3
[PATCH 4/4] ibmvfc: Clean up transport events
No change to functionality. Simply make transport event messages a litle
clearer, and rework CRQ format enums such that we have separate enums
for INIT messages and XPORT events.
Signed-off-by: Tyrel Datwyler
---
drivers/scsi/ibmvscsi/ibmvfc.c | 8 +---
drivers/scsi/ibmvscsi/ibmvfc.h | 7 ++-
2 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c
index 33dda4d32f65..3ad997ac3510 100644
--- a/drivers/scsi/ibmvscsi/ibmvfc.c
+++ b/drivers/scsi/ibmvscsi/ibmvfc.c
@@ -2756,16 +2756,18 @@ static void ibmvfc_handle_crq(struct ibmvfc_crq *crq,
struct ibmvfc_host *vhost)
ibmvfc_set_host_action(vhost, IBMVFC_HOST_ACTION_NONE);
if (crq->format == IBMVFC_PARTITION_MIGRATED) {
/* We need to re-setup the interpartition connection */
- dev_info(vhost->dev, "Re-enabling adapter\n");
+ dev_info(vhost->dev, "Partition migrated, Re-enabling
adapter\n");
vhost->client_migrated = 1;
ibmvfc_purge_requests(vhost, DID_REQUEUE);
ibmvfc_link_down(vhost, IBMVFC_LINK_DOWN);
ibmvfc_set_host_action(vhost,
IBMVFC_HOST_ACTION_REENABLE);
- } else {
- dev_err(vhost->dev, "Virtual adapter failed (rc=%d)\n",
crq->format);
+ } else if (crq->format == IBMVFC_PARTNER_FAILED || crq->format
== IBMVFC_PARTNER_DEREGISTER) {
+ dev_err(vhost->dev, "Host partner adapter deregistered
or failed (rc=%d)\n", crq->format);
ibmvfc_purge_requests(vhost, DID_ERROR);
ibmvfc_link_down(vhost, IBMVFC_LINK_DOWN);
ibmvfc_set_host_action(vhost, IBMVFC_HOST_ACTION_RESET);
+ } else {
+ dev_err(vhost->dev, "Received unknown transport event
from partner (rc=%d)\n", crq->format);
}
return;
case IBMVFC_CRQ_CMD_RSP:
diff --git a/drivers/scsi/ibmvscsi/ibmvfc.h b/drivers/scsi/ibmvscsi/ibmvfc.h
index b81a53c4a9a8..459cc288ba1d 100644
--- a/drivers/scsi/ibmvscsi/ibmvfc.h
+++ b/drivers/scsi/ibmvscsi/ibmvfc.h
@@ -78,9 +78,14 @@ enum ibmvfc_crq_valid {
IBMVFC_CRQ_XPORT_EVENT = 0xFF,
};
-enum ibmvfc_crq_format {
+enum ibmvfc_crq_init_msg {
IBMVFC_CRQ_INIT = 0x01,
IBMVFC_CRQ_INIT_COMPLETE= 0x02,
+};
+
+enum ibmvfc_crq_xport_evts {
+ IBMVFC_PARTNER_FAILED = 0x01,
+ IBMVFC_PARTNER_DEREGISTER = 0x02,
IBMVFC_PARTITION_MIGRATED = 0x06,
};
--
2.12.3
[PATCH 3/4] ibmvfc: Byte swap status and error codes when logging
Status and error codes are returned in big endian from the VIOS. The values are translated into a human readable format when logged, but the values are also logged. This patch byte swaps those values so that they are consistent between BE and LE platforms. Signed-off-by: Tyrel Datwyler --- drivers/scsi/ibmvscsi/ibmvfc.c | 28 +++- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c index 18ee2a8ec3d5..33dda4d32f65 100644 --- a/drivers/scsi/ibmvscsi/ibmvfc.c +++ b/drivers/scsi/ibmvscsi/ibmvfc.c @@ -1497,7 +1497,7 @@ static void ibmvfc_log_error(struct ibmvfc_event *evt) scmd_printk(KERN_ERR, cmnd, "Command (%02X) : %s (%x:%x) " "flags: %x fcp_rsp: %x, resid=%d, scsi_status: %x\n", - cmnd->cmnd[0], err, vfc_cmd->status, vfc_cmd->error, + cmnd->cmnd[0], err, be16_to_cpu(vfc_cmd->status), be16_to_cpu(vfc_cmd->error), rsp->flags, rsp_code, scsi_get_resid(cmnd), rsp->scsi_status); } @@ -2023,7 +2023,7 @@ static int ibmvfc_reset_device(struct scsi_device *sdev, int type, char *desc) sdev_printk(KERN_ERR, sdev, "%s reset failed: %s (%x:%x) " "flags: %x fcp_rsp: %x, scsi_status: %x\n", desc, ibmvfc_get_cmd_error(be16_to_cpu(rsp_iu.cmd.status), be16_to_cpu(rsp_iu.cmd.error)), - rsp_iu.cmd.status, rsp_iu.cmd.error, fc_rsp->flags, rsp_code, + be16_to_cpu(rsp_iu.cmd.status), be16_to_cpu(rsp_iu.cmd.error), fc_rsp->flags, rsp_code, fc_rsp->scsi_status); rsp_rc = -EIO; } else @@ -2382,7 +2382,7 @@ static int ibmvfc_abort_task_set(struct scsi_device *sdev) sdev_printk(KERN_ERR, sdev, "Abort failed: %s (%x:%x) " "flags: %x fcp_rsp: %x, scsi_status: %x\n", ibmvfc_get_cmd_error(be16_to_cpu(rsp_iu.cmd.status), be16_to_cpu(rsp_iu.cmd.error)), - rsp_iu.cmd.status, rsp_iu.cmd.error, fc_rsp->flags, rsp_code, + be16_to_cpu(rsp_iu.cmd.status), be16_to_cpu(rsp_iu.cmd.error), fc_rsp->flags, rsp_code, fc_rsp->scsi_status); rsp_rc = -EIO; } else @@ -3349,7 +3349,7 @@ static void ibmvfc_tgt_prli_done(struct ibmvfc_event *evt) tgt_log(tgt, level, "Process Login failed: %s (%x:%x) rc=0x%02X\n", ibmvfc_get_cmd_error(be16_to_cpu(rsp->status), be16_to_cpu(rsp->error)), - rsp->status, rsp->error, status); + be16_to_cpu(rsp->status), be16_to_cpu(rsp->error), status); break; } @@ -3447,9 +3447,10 @@ static void ibmvfc_tgt_plogi_done(struct ibmvfc_event *evt) ibmvfc_set_tgt_action(tgt, IBMVFC_TGT_ACTION_DEL_RPORT); tgt_log(tgt, level, "Port Login failed: %s (%x:%x) %s (%x) %s (%x) rc=0x%02X\n", - ibmvfc_get_cmd_error(be16_to_cpu(rsp->status), be16_to_cpu(rsp->error)), rsp->status, rsp->error, - ibmvfc_get_fc_type(be16_to_cpu(rsp->fc_type)), rsp->fc_type, - ibmvfc_get_ls_explain(be16_to_cpu(rsp->fc_explain)), rsp->fc_explain, status); + ibmvfc_get_cmd_error(be16_to_cpu(rsp->status), be16_to_cpu(rsp->error)), +be16_to_cpu(rsp->status), be16_to_cpu(rsp->error), + ibmvfc_get_fc_type(be16_to_cpu(rsp->fc_type)), be16_to_cpu(rsp->fc_type), + ibmvfc_get_ls_explain(be16_to_cpu(rsp->fc_explain)), be16_to_cpu(rsp->fc_explain), status); break; } @@ -3620,7 +3621,7 @@ static void ibmvfc_tgt_adisc_done(struct ibmvfc_event *evt) fc_explain = (be32_to_cpu(mad->fc_iu.response[1]) & 0xff00) >> 8; tgt_info(tgt, "ADISC failed: %s (%x:%x) %s (%x) %s (%x) rc=0x%02X\n", ibmvfc_get_cmd_error(be16_to_cpu(mad->iu.status), be16_to_cpu(mad->iu.error)), -mad->iu.status, mad->iu.error, +be16_to_cpu(mad->iu.status), be16_to_cpu(mad->iu.error), ibmvfc_get_fc_type(fc_reason), fc_reason, ibmvfc_get_ls_explain(fc_explain), fc_explain, status); break; @@ -3832,9 +3833,10 @@ static void ibmvfc_tgt_query_target_done(struct ibmvfc_event *evt) tgt_log(tgt, level, "Query Target failed: %s (%x:%x) %s (%x) %s (%x) rc=0x%02X\n",
[PATCH 2/4] ibmvfc: Add failed PRLI to cmd_status lookup array
The VIOS uses the SCSI_ERROR class to report PRLI failures. These
errors are indicated with the combination of a IBMVFC_FC_SCSI_ERROR
return status and 0x8000 error code. Add these codes to cmd_status[]
with appropriate human readable error message.
Signed-off-by: Tyrel Datwyler
---
drivers/scsi/ibmvscsi/ibmvfc.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c
index c3ce27039552..18ee2a8ec3d5 100644
--- a/drivers/scsi/ibmvscsi/ibmvfc.c
+++ b/drivers/scsi/ibmvscsi/ibmvfc.c
@@ -139,6 +139,7 @@ static const struct {
{ IBMVFC_FC_FAILURE, IBMVFC_VENDOR_SPECIFIC, DID_ERROR, 1, 1, "vendor
specific" },
{ IBMVFC_FC_SCSI_ERROR, 0, DID_OK, 1, 0, "SCSI error" },
+ { IBMVFC_FC_SCSI_ERROR, IBMVFC_COMMAND_FAILED, DID_ERROR, 0, 1, "PRLI
to device failed." },
};
static void ibmvfc_npiv_login(struct ibmvfc_host *);
--
2.12.3
[PATCH 1/4] ibmvfc: Remove "failed" from logged errors
The text of messages logged with ibmvfc_log_error() always contain the term "failed". In the case of cancelled commands during EH they are reported back by the VIOS using error codes. This can be confusing to somebody looking at these log messages as to whether a command was successfully cancelled. The following real log message for example it is unclear if the transaction was actaully cancelled. <6>sd 0:0:1:1: Cancelling outstanding commands. <3>sd 0:0:1:1: [sde] Command (28) failed: transaction cancelled (2:6) flags: 0 fcp_rsp: 0, resid=0, scsi_status: 0 Remove prefixing of "failed" to all error logged messages. The ibmvfc_log_error() function translates the returned error/status codes to a human readable message already. Signed-off-by: Tyrel Datwyler --- drivers/scsi/ibmvscsi/ibmvfc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c index dbaa4f131433..c3ce27039552 100644 --- a/drivers/scsi/ibmvscsi/ibmvfc.c +++ b/drivers/scsi/ibmvscsi/ibmvfc.c @@ -1494,7 +1494,7 @@ static void ibmvfc_log_error(struct ibmvfc_event *evt) if (rsp->flags & FCP_RSP_LEN_VALID) rsp_code = rsp->data.info.rsp_code; - scmd_printk(KERN_ERR, cmnd, "Command (%02X) failed: %s (%x:%x) " + scmd_printk(KERN_ERR, cmnd, "Command (%02X) : %s (%x:%x) " "flags: %x fcp_rsp: %x, resid=%d, scsi_status: %x\n", cmnd->cmnd[0], err, vfc_cmd->status, vfc_cmd->error, rsp->flags, rsp_code, scsi_get_resid(cmnd), rsp->scsi_status); -- 2.12.3
[PATCH 2/2] ibmvscsi: Fix empty event pool access during host removal
The event pool used for queueing commands is destroyed fairly early in
the ibmvscsi_remove() code path. Since, this happens prior to the call
so scsi_remove_host() it is possible for further calls to queuecommand
to be processed which manifest as a panic due to a NULL pointer
dereference as seen here:
PANIC: "Unable to handle kernel paging request for data at address
0x"
Context process backtrace:
DSISR: 4200 Syscall Result:
4 [c2cb3820] memcpy_power7 at c0064204
[Link Register] [c2cb3820] ibmvscsi_send_srp_event at d3ed14a4
5 [c2cb3920] ibmvscsi_send_srp_event at d3ed14a4 [ibmvscsi]
?(unreliable)
6 [c2cb39c0] ibmvscsi_queuecommand at d3ed2388 [ibmvscsi]
7 [c2cb3a70] scsi_dispatch_cmd at d395c2d8 [scsi_mod]
8 [c2cb3af0] scsi_request_fn at d395ef88 [scsi_mod]
9 [c2cb3be0] __blk_run_queue at c0429860
10 [c2cb3c10] blk_delay_work at c042a0ec
11 [c2cb3c40] process_one_work at c00dac30
12 [c2cb3cd0] worker_thread at c00db110
13 [c2cb3d80] kthread at c00e3378
14 [c2cb3e30] ret_from_kernel_thread at c000982c
The kernel buffer log is overfilled with this log:
[11261.952732] ibmvscsi: found no event struct in pool!
This patch reorders the operations during host teardown. Start by
calling the SRP transport and Scsi_Host remove functions to flush any
outstanding work and set the host offline. LLDD teardown follows
including destruction of the event pool, freeing the Command Response
Queue (CRQ), and unmapping any persistent buffers. The event pool
destruction is protected by the scsi_host lock, and the pool is purged
prior of any requests for which we never received a response. Finally,
move the removal of the scsi host from our global list to the end so
that the host is easily locatable for debugging purposes during
teardown.
Cc: # v2.6.12+
Signed-off-by: Tyrel Datwyler
---
drivers/scsi/ibmvscsi/ibmvscsi.c | 22 --
1 file changed, 16 insertions(+), 6 deletions(-)
diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
index 2b22969f3f63..8cec5230fe31 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.c
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
@@ -2295,17 +2295,27 @@ static int ibmvscsi_probe(struct vio_dev *vdev, const
struct vio_device_id *id)
static int ibmvscsi_remove(struct vio_dev *vdev)
{
struct ibmvscsi_host_data *hostdata = dev_get_drvdata(&vdev->dev);
- spin_lock(&ibmvscsi_driver_lock);
- list_del(&hostdata->host_list);
- spin_unlock(&ibmvscsi_driver_lock);
- unmap_persist_bufs(hostdata);
+ unsigned long flags;
+
+ srp_remove_host(hostdata->host);
+ scsi_remove_host(hostdata->host);
+
+ purge_requests(hostdata, DID_ERROR);
+
+ spin_lock_irqsave(hostdata->host->host_lock, flags);
release_event_pool(&hostdata->pool, hostdata);
+ spin_unlock_irqrestore(hostdata->host->host_lock, flags);
+
ibmvscsi_release_crq_queue(&hostdata->queue, hostdata,
max_events);
kthread_stop(hostdata->work_thread);
- srp_remove_host(hostdata->host);
- scsi_remove_host(hostdata->host);
+ unmap_persist_bufs(hostdata);
+
+ spin_lock(&ibmvscsi_driver_lock);
+ list_del(&hostdata->host_list);
+ spin_unlock(&ibmvscsi_driver_lock);
+
scsi_host_put(hostdata->host);
return 0;
--
2.12.3
[PATCH 1/2] ibmvscsi: Protect ibmvscsi_head from concurrent modificaiton
For each ibmvscsi host created during a probe or destroyed during a
remove we either add or remove that host to/from the global ibmvscsi_head
list. This runs the risk of concurrent modification.
This patch adds a simple spinlock around the list modification calls to
prevent concurrent updates as is done similarly in the ibmvfc driver and
ipr driver.
Fixes: 32d6e4b6e4ea ("scsi: ibmvscsi: add vscsi hosts to global list_head")
Cc: # v4.10+
Signed-off-by: Tyrel Datwyler
---
drivers/scsi/ibmvscsi/ibmvscsi.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
index 1135e74646e2..2b22969f3f63 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.c
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
@@ -96,6 +96,7 @@ static int client_reserve = 1;
static char partition_name[96] = "UNKNOWN";
static unsigned int partition_number = -1;
static LIST_HEAD(ibmvscsi_head);
+static DEFINE_SPINLOCK(ibmvscsi_driver_lock);
static struct scsi_transport_template *ibmvscsi_transport_template;
@@ -2270,7 +2271,9 @@ static int ibmvscsi_probe(struct vio_dev *vdev, const
struct vio_device_id *id)
}
dev_set_drvdata(&vdev->dev, hostdata);
+ spin_lock(&ibmvscsi_driver_lock);
list_add_tail(&hostdata->host_list, &ibmvscsi_head);
+ spin_unlock(&ibmvscsi_driver_lock);
return 0;
add_srp_port_failed:
@@ -2292,7 +2295,9 @@ static int ibmvscsi_probe(struct vio_dev *vdev, const
struct vio_device_id *id)
static int ibmvscsi_remove(struct vio_dev *vdev)
{
struct ibmvscsi_host_data *hostdata = dev_get_drvdata(&vdev->dev);
+ spin_lock(&ibmvscsi_driver_lock);
list_del(&hostdata->host_list);
+ spin_unlock(&ibmvscsi_driver_lock);
unmap_persist_bufs(hostdata);
release_event_pool(&hostdata->pool, hostdata);
ibmvscsi_release_crq_queue(&hostdata->queue, hostdata,
--
2.12.3
Re: [PATCH v2 2/2] arch: powerpc/hlwd-pic: Add missing of_node-put() in hlwd_pic_probe()
On 02/25/2019 01:00 AM, Himadri Pandya wrote: > Decrement the reference count on device_node "np" while breaking out of > the loop. Issue identified by Coccinelle. > > Signed-off-by: Himadri Pandya > --- > Changes in V2: > - Change subject line > --- > arch/powerpc/platforms/embedded6xx/hlwd-pic.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/powerpc/platforms/embedded6xx/hlwd-pic.c > b/arch/powerpc/platforms/embedded6xx/hlwd-pic.c > index 8112b39879d6..3ee1404ceef4 100644 > --- a/arch/powerpc/platforms/embedded6xx/hlwd-pic.c > +++ b/arch/powerpc/platforms/embedded6xx/hlwd-pic.c > @@ -220,6 +220,7 @@ void hlwd_pic_probe(void) > irq_set_chained_handler(cascade_virq, > hlwd_pic_irq_cascade); > hlwd_irq_host = host; > + of_node_put(np); > break; > } > } > @@ -237,4 +238,3 @@ void hlwd_quiesce(void) > > __hlwd_quiesce(io_base); > } > - > This line removal at the end of the patch is likely accidental, but really should not be here. Otherwise, Reviewed-by: Tyrel Datwyler
Re: [PATCH v2 1/2] arch: powerpc/kexec: Add missing of_node-put() in default_machine_kexec_prepare()
On 02/25/2019 01:00 AM, Himadri Pandya wrote:
> Decrement the reference count on device_node "node" while breaking out
> of the loop. Issue identified by Coccinelle.
>
> Signed-off-by: Himadri Pandya
> ---
> Changes in V2:
> - Change subject line
> ---
> arch/powerpc/kernel/machine_kexec_64.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/powerpc/kernel/machine_kexec_64.c
> b/arch/powerpc/kernel/machine_kexec_64.c
> index a0f6f45005bd..46edde582984 100644
> --- a/arch/powerpc/kernel/machine_kexec_64.c
> +++ b/arch/powerpc/kernel/machine_kexec_64.c
> @@ -65,6 +65,7 @@ int default_machine_kexec_prepare(struct kimage *image)
> end = begin + image->segment[i].memsz;
>
> if ((begin < high) && (end > low))
> + of_node_put(node);
> return -ETXTBSY;
I think the kbuild robot already caught this, but you need to add { } block
around this if now. Previously it was a one line statement, but adding the
of_node_put() makes it multiple lines that now require { }.
-Tyrel
> }
> }
>
Re: [PATCH] powerpc/pseries: Fix dn reference error in dlpar_cpu_remove_by_index
On 02/19/2019 07:46 AM, Michael Bringmann wrote:
> powerpc/pseries: Fix dn reference error in dlpar_cpu_remove_by_index()
>
> A reference to the device node of the CPU to be removed is released
> upon successful removal of the associated CPU device. If the call
> to remove the CPU device fails, dlpar_cpu_remove_by_index() still
> frees the reference and this leads to miscomparisons and/or
> addressing errors later on.
>
> This problem may be observed when trying to DLPAR 'hot-remove' a CPU
> from a system that has only a single CPU. The operation will fail
> because there is no other CPU to which the kernel operations may be
> migrated, but the refcount will still be decremented.
>
> Signed-off-by: Michael Bringmann
>
>
> diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c
> b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> index 97feb6e..9537bb9 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> @@ -635,7 +635,8 @@ static int dlpar_cpu_remove_by_index(u32 drc_index)
> }
>
> rc = dlpar_cpu_remove(dn, drc_index);
> - of_node_put(dn);
> + if (!rc)
> + of_node_put(dn);
> return rc;
> }
>
NACK!
The logic here is wrong. Here is the full function.
static int dlpar_cpu_remove_by_index(u32 drc_index)
{
struct device_node *dn;
int rc;
dn = cpu_drc_index_to_dn(drc_index);
if (!dn) {
pr_warn("Cannot find CPU (drc index %x) to remove\n",
drc_index);
return -ENODEV;
}
rc = dlpar_cpu_remove(dn, drc_index);
of_node_put(dn);
return rc;
}
The call to cpu_drc_index_to_dn() returns a device_node with the reference count
incremented. So, regardless of the success or failure of the call to
dlpar_cpu_remove() you need to release that reference.
If there is a reference counting issue it is somewhere else.
-Tyrel
Re: [PATCH] powerpc/pseries: export timebase register sample in lparcfg
Ping? Any objections to this patch? A fix is already upstream in powerpc-utils
to utilize the timebase value if available.
-Tyrel
On 12/08/2018 03:48 PM, Tyrel Datwyler wrote:
> The Processor Utilzation of Resource Registers (PURR) provide an estimate of
> resources used by a cpu thread. Section 7.6 in Book III of the ISA outlines
> how to calculate the percentage of shared resources for threads using the
> ratio of the PURR delta and Timebase Register delta for a sampled period.
>
> This calculation is currently done erroneously by the lparstat tool from the
> powerpc-utils package. This patch exports the current timebase value after
> we sample the PURRs and exposes it to userspace accounting tools via
> /proc/ppc64/lparcfg.
>
> Signed-off-by: Tyrel Datwyler
> ---
> arch/powerpc/platforms/pseries/lparcfg.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/powerpc/platforms/pseries/lparcfg.c
> b/arch/powerpc/platforms/pseries/lparcfg.c
> index 7944873..1ea73ec 100644
> --- a/arch/powerpc/platforms/pseries/lparcfg.c
> +++ b/arch/powerpc/platforms/pseries/lparcfg.c
> @@ -475,6 +475,7 @@ static int pseries_lparcfg_data(struct seq_file *m, void
> *v)
> splpar_dispatch_data(m);
>
> seq_printf(m, "purr=%ld\n", get_purr());
> +seq_printf(m, "tbr=%ld\n", mftb());
> } else {/* non SPLPAR case */
>
> seq_printf(m, "system_active_processors=%d\n",
>
Re: [PATCH v02] powerpc/pseries: Check for ceded CPU's during LPAR migration
On 01/31/2019 02:21 PM, Tyrel Datwyler wrote: > On 01/31/2019 01:53 PM, Michael Bringmann wrote: >> On 1/30/19 11:38 PM, Michael Ellerman wrote: >>> Michael Bringmann writes: >>>> This patch is to check for cede'ed CPUs during LPM. Some extreme >>>> tests encountered a problem ehere Linux has put some threads to >>>> sleep (possibly to save energy or something), LPM was attempted, >>>> and the Linux kernel didn't awaken the sleeping threads, but issued >>>> the H_JOIN for the active threads. Since the sleeping threads >>>> are not awake, they can not issue the expected H_JOIN, and the >>>> partition would never suspend. This patch wakes the sleeping >>>> threads back up. >>> >>> I'm don't think this is the right solution. >>> >>> Just after your for loop we do an on_each_cpu() call, which sends an IPI >>> to every CPU, and that should wake all CPUs up from CEDE. >>> >>> If that's not happening then there is a bug somewhere, and we need to >>> work out where. >> >> Let me explain the scenario of the LPM case that Pete Heyrman found, and >> that Nathan F. was working upon, previously. >> >> In the scenario, the partition has 5 dedicated processors each with 8 threads >> running. > > Do we CEDE processors when running dedicated? I thought H_CEDE was part of the > Shared Processor LPAR option. Looks like the cpuidle-pseries driver uses CEDE with dedicated processors as long as firmware supports SPLPAR option. > >> >> From the PHYP data we can see that on VP 0, threads 3, 4, 5, 6 and 7 issued >> a H_CEDE requesting to save energy by putting the requesting thread into >> sleep mode. In this state, the thread will only be awakened by H_PROD from >> another running thread or from an external user action (power off, reboot >> and such). Timers and external interrupts are disabled in this mode. > > Not according to PAPR. A CEDE'd processor should awaken if signaled by > external > interrupt such as decrementer or IPI as well. This statement should still apply though. From PAPR: 14.11.3.3 H_CEDE The architectural intent of this hcall() is to have the virtual processor, which has no useful work to do, enter a wait state ceding its processor capacity to other virtual processors until some useful work appears, signaled either through an interrupt or a prod hcall(). To help the caller reduce race conditions, this call may be made with interrupts disabled but the semantics of the hcall() enable the virtual processor’s interrupts so that it may always receive wake up interrupt signals. -Tyrel > > -Tyrel > >> >> About 3 seconds later, as part of the LPM operation, the other 35 threads >> have all issued a H_JOIN request. Join is part of the LPM process where >> the threads suspend themselves as part of the LPM operation so the partition >> can be migrated to the target server. >> >> So, the current state is the the OS has suspended the execution of all the >> threads in the partition without successfully suspending all threads as part >> of LPM. >> >> Net, OS has an issue where they suspended every processor thread so nothing >> can run. >> >> This appears to be slightly different than the previous LPM stalls we have >> seen where the migration stalls because of cpus being taken offline and not >> making the H_JOIN call. >> >> In this scenario we appear to have CPUs that have done an H_CEDE prior to >> the LPM. For these CPUs we would need to do a H_PROD to wake them back up >> so they can do a H_JOIN and allow the LPM to continue. >> >> The problem is that Linux has some threads that they put to sleep (probably >> to save energy or something), LPM was attempted, Linux didn't awaken the >> sleeping threads but issued the H_JOIN for the active threads. Since the >> sleeping threads don't issue the H_JOIN the partition will never suspend. >> >> I am checking again with Pete regarding your concerns. >> >> Thanks. >> >>> >>> >>>> diff --git a/arch/powerpc/include/asm/plpar_wrappers.h >>>> b/arch/powerpc/include/asm/plpar_wrappers.h >>>> index cff5a41..8292eff 100644 >>>> --- a/arch/powerpc/include/asm/plpar_wrappers.h >>>> +++ b/arch/powerpc/include/asm/plpar_wrappers.h >>>> @@ -26,10 +26,8 @@ static inline void set_cede_latency_hint(u8 >>>> latency_hint) >>>>get_lppaca()->cede_latency_hint = latency_hint; >>>> } >>>> >>>> -static i
Re: [PATCH v02] powerpc/pseries: Check for ceded CPU's during LPAR migration
On 01/31/2019 01:53 PM, Michael Bringmann wrote:
> On 1/30/19 11:38 PM, Michael Ellerman wrote:
>> Michael Bringmann writes:
>>> This patch is to check for cede'ed CPUs during LPM. Some extreme
>>> tests encountered a problem ehere Linux has put some threads to
>>> sleep (possibly to save energy or something), LPM was attempted,
>>> and the Linux kernel didn't awaken the sleeping threads, but issued
>>> the H_JOIN for the active threads. Since the sleeping threads
>>> are not awake, they can not issue the expected H_JOIN, and the
>>> partition would never suspend. This patch wakes the sleeping
>>> threads back up.
>>
>> I'm don't think this is the right solution.
>>
>> Just after your for loop we do an on_each_cpu() call, which sends an IPI
>> to every CPU, and that should wake all CPUs up from CEDE.
>>
>> If that's not happening then there is a bug somewhere, and we need to
>> work out where.
>
> Let me explain the scenario of the LPM case that Pete Heyrman found, and
> that Nathan F. was working upon, previously.
>
> In the scenario, the partition has 5 dedicated processors each with 8 threads
> running.
Do we CEDE processors when running dedicated? I thought H_CEDE was part of the
Shared Processor LPAR option.
>
> From the PHYP data we can see that on VP 0, threads 3, 4, 5, 6 and 7 issued
> a H_CEDE requesting to save energy by putting the requesting thread into
> sleep mode. In this state, the thread will only be awakened by H_PROD from
> another running thread or from an external user action (power off, reboot
> and such). Timers and external interrupts are disabled in this mode.
Not according to PAPR. A CEDE'd processor should awaken if signaled by external
interrupt such as decrementer or IPI as well.
-Tyrel
>
> About 3 seconds later, as part of the LPM operation, the other 35 threads
> have all issued a H_JOIN request. Join is part of the LPM process where
> the threads suspend themselves as part of the LPM operation so the partition
> can be migrated to the target server.
>
> So, the current state is the the OS has suspended the execution of all the
> threads in the partition without successfully suspending all threads as part
> of LPM.
>
> Net, OS has an issue where they suspended every processor thread so nothing
> can run.
>
> This appears to be slightly different than the previous LPM stalls we have
> seen where the migration stalls because of cpus being taken offline and not
> making the H_JOIN call.
>
> In this scenario we appear to have CPUs that have done an H_CEDE prior to
> the LPM. For these CPUs we would need to do a H_PROD to wake them back up
> so they can do a H_JOIN and allow the LPM to continue.
>
> The problem is that Linux has some threads that they put to sleep (probably
> to save energy or something), LPM was attempted, Linux didn't awaken the
> sleeping threads but issued the H_JOIN for the active threads. Since the
> sleeping threads don't issue the H_JOIN the partition will never suspend.
>
> I am checking again with Pete regarding your concerns.
>
> Thanks.
>
>>
>>
>>> diff --git a/arch/powerpc/include/asm/plpar_wrappers.h
>>> b/arch/powerpc/include/asm/plpar_wrappers.h
>>> index cff5a41..8292eff 100644
>>> --- a/arch/powerpc/include/asm/plpar_wrappers.h
>>> +++ b/arch/powerpc/include/asm/plpar_wrappers.h
>>> @@ -26,10 +26,8 @@ static inline void set_cede_latency_hint(u8 latency_hint)
>>> get_lppaca()->cede_latency_hint = latency_hint;
>>> }
>>>
>>> -static inline long cede_processor(void)
>>> -{
>>> - return plpar_hcall_norets(H_CEDE);
>>> -}
>>> +int cpu_is_ceded(int cpu);
>>> +long cede_processor(void);
>>>
>>> static inline long extended_cede_processor(unsigned long latency_hint)
>>> {
>>> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
>>> index de35bd8f..fea3d21 100644
>>> --- a/arch/powerpc/kernel/rtas.c
>>> +++ b/arch/powerpc/kernel/rtas.c
>>> @@ -44,6 +44,7 @@
>>> #include
>>> #include
>>> #include
>>> +#include
>>>
>>> /* This is here deliberately so it's only used in this file */
>>> void enter_rtas(unsigned long);
>>> @@ -942,7 +943,7 @@ int rtas_ibm_suspend_me(u64 handle)
>>> struct rtas_suspend_me_data data;
>>> DECLARE_COMPLETION_ONSTACK(done);
>>> cpumask_var_t offline_mask;
>>> - int cpuret;
>>> + int cpuret, cpu;
>>>
>>> if (!rtas_service_present("ibm,suspend-me"))
>>> return -ENOSYS;
>>> @@ -991,6 +992,11 @@ int rtas_ibm_suspend_me(u64 handle)
>>> goto out_hotplug_enable;
>>> }
>>>
>>> + for_each_present_cpu(cpu) {
>>> + if (cpu_is_ceded(cpu))
>>> + plpar_hcall_norets(H_PROD,
>>> get_hard_smp_processor_id(cpu));
>>> + }
>>
>> There's a race condition here, there's nothing to prevent the CPUs you
>> just PROD'ed from going back into CEDE before you do the on_each_cpu()
>> call below>
>>> /* Call function on all CPUs. One of us will make the
>>> * rtas call
>>> */
>>> diff --git a/arch/p
Re: [RFC 5/6] powerpc/pci/hotplug: Use common drcinfo parsing
On 01/14/2019 04:28 PM, Bjorn Helgaas wrote:
> On Fri, Dec 14, 2018 at 02:51:31PM -0600, Michael Bringmann wrote:
>> The implementation of the pseries-specific drc info properties
>> is currently implemented in pseries-specific and non-pseries-specific
>> files. This patch set uses a new implementation of the device-tree
>> parsing code for the properties.
>>
>> This patch refactors parsing of the pseries-specific drc-info properties
>> out of rpaphp_core.c to use the common parser. In the case where an
>> architecture does not use these properties, an __weak copy of the
>> function is provided with dummy return values. Changes include creating
>> appropriate callback functions and passing callback-specific data
>> blocks into arch_find_drc_match. All functions that were used just
>> to support the previous parsing implementation have been moved.
>>
>> Signed-off-by: Michael Bringmann
>
> This is fine with me. Any rpaphp_core.c maintainers want to comment?
> Tyrel?
It greatly simplifies the code in rpaphp_core.c, and as far as I can tell the
refactoring maintains the existing functionality.
Acked-by: Tyrel Datwyler
>
> $ ./scripts/get_maintainer.pl -f drivers/pci/hotplug/rpaphp_core.c
> Tyrel Datwyler (supporter:IBM Power PCI Hotplug
> Driver for RPA-compliant...)
> Benjamin Herrenschmidt (supporter:LINUX FOR
> POWERPC (32-BIT AND 64-BIT))
> Paul Mackerras (supporter:LINUX FOR POWERPC (32-BIT AND
> 64-BIT))
> Michael Ellerman (supporter:LINUX FOR POWERPC (32-BIT
> AND 64-BIT))
>
>> ---
>> drivers/pci/hotplug/rpaphp_core.c | 232
>> -
>> 1 file changed, 28 insertions(+), 204 deletions(-)
>>
>> diff --git a/drivers/pci/hotplug/rpaphp_core.c
>> b/drivers/pci/hotplug/rpaphp_core.c
>> index bcd5d35..9ad7384 100644
>> --- a/drivers/pci/hotplug/rpaphp_core.c
>> +++ b/drivers/pci/hotplug/rpaphp_core.c
>> @@ -154,182 +154,18 @@ static enum pci_bus_speed get_max_bus_speed(struct
>> slot *slot)
>> return speed;
>> }
>>
>> -static int get_children_props(struct device_node *dn, const int
>> **drc_indexes,
>> -const int **drc_names, const int **drc_types,
>> -const int **drc_power_domains)
>> -{
>> -const int *indexes, *names, *types, *domains;
>> -
>> -indexes = of_get_property(dn, "ibm,drc-indexes", NULL);
>> -names = of_get_property(dn, "ibm,drc-names", NULL);
>> -types = of_get_property(dn, "ibm,drc-types", NULL);
>> -domains = of_get_property(dn, "ibm,drc-power-domains", NULL);
>> -
>> -if (!indexes || !names || !types || !domains) {
>> -/* Slot does not have dynamically-removable children */
>> -return -EINVAL;
>> -}
>> -if (drc_indexes)
>> -*drc_indexes = indexes;
>> -if (drc_names)
>> -/* &drc_names[1] contains NULL terminated slot names */
>> -*drc_names = names;
>> -if (drc_types)
>> -/* &drc_types[1] contains NULL terminated slot types */
>> -*drc_types = types;
>> -if (drc_power_domains)
>> -*drc_power_domains = domains;
>> -
>> -return 0;
>> -}
>> -
>> -
>> /* Verify the existence of 'drc_name' and/or 'drc_type' within the
>> - * current node. First obtain it's my-drc-index property. Next,
>> - * obtain the DRC info from it's parent. Use the my-drc-index for
>> - * correlation, and obtain/validate the requested properties.
>> + * current node.
>> */
>>
>> -static int rpaphp_check_drc_props_v1(struct device_node *dn, char *drc_name,
>> -char *drc_type, unsigned int my_index)
>> -{
>> -char *name_tmp, *type_tmp;
>> -const int *indexes, *names;
>> -const int *types, *domains;
>> -int i, rc;
>> -
>> -rc = get_children_props(dn->parent, &indexes, &names, &types, &domains);
>> -if (rc < 0) {
>> -return -EINVAL;
>> -}
>> -
>> -name_tmp = (char *) &names[1];
>> -type_tmp = (char *) &types[1];
>> -
>> -/* Iterate through parent properties, looking for my-drc-index */
>> -for (i = 0; i < be32_to_cpu(indexes[0]); i++) {
>> -if ((unsigned int) indexes[i + 1] == my_index)
>> -break;
>> -
>> -name_tmp += (strlen(name_tmp) + 1);
>> -type_tmp += (strlen(type_
Re: [RFC 1/6] powerpc:/drc Define interface to acquire arch-specific drc info
On 12/14/2018 12:50 PM, Michael Bringmann wrote: > Define interface to acquire arch-specific drc info to match against > hotpluggable devices. The current implementation exposes several > pseries-specific dynamic memory properties in generic kernel code. > This patch set provides an interface to pull that code out of the > generic kernel. > > Signed-off-by: Michael Bringmann > --- > include/linux/topology.h |9 + > 1 file changed, 9 insertions(+) > > diff --git a/include/linux/topology.h b/include/linux/topology.h > index cb0775e..df97f5f 100644 > --- a/include/linux/topology.h > +++ b/include/linux/topology.h > @@ -44,6 +44,15 @@ > > int arch_update_cpu_topology(void); On another note a kern doc comment for this function would also be nice. -Tyrel > > +int arch_find_drc_match(struct device_node *dn, > + bool (*usercb)(struct device_node *dn, > + u32 drc_index, char *drc_name, > + char *drc_type, u32 drc_power_domain, > + void *data), > + char *opt_drc_type, char *opt_drc_name, > + bool match_drc_index, bool ck_php_type, > + void *data); > + > /* Conform to ACPI 2.0 SLIT distance definitions */ > #define LOCAL_DISTANCE 10 > #define REMOTE_DISTANCE 20 >
Re: [RFC 3/6] pseries/drcinfo: Pseries impl of arch_find_drc_info
On 12/14/2018 12:51 PM, Michael Bringmann wrote:
> This patch provides a common interface to parse ibm,drc-indexes,
> ibm,drc-names, ibm,drc-types, ibm,drc-power-domains, or ibm,drc-info.
> The generic interface arch_find_drc_match is provided which accepts
> callback functions that may be applied to examine the data for each
> entry.
>
The title of your patch is "pseries/drcinfo: Pseries impl of arch_find_drc_info"
but the name of the function you are ultimately implementing is
arch_find_drc_match if I'm not mistaken.
> Signed-off-by: Michael Bringmann
> ---
> arch/powerpc/include/asm/prom.h |3
> arch/powerpc/platforms/pseries/of_helpers.c | 299
> +++
> include/linux/topology.h|2
> 3 files changed, 298 insertions(+), 6 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/prom.h b/arch/powerpc/include/asm/prom.h
> index b04c5ce..910d1dc 100644
> --- a/arch/powerpc/include/asm/prom.h
> +++ b/arch/powerpc/include/asm/prom.h
> @@ -91,9 +91,6 @@ struct of_drc_info {
> u32 last_drc_index;
> };
>
> -extern int of_read_drc_info_cell(struct property **prop,
> - const __be32 **curval, struct of_drc_info *data);
> -
>
> /*
> * There are two methods for telling firmware what our capabilities are.
> diff --git a/arch/powerpc/platforms/pseries/of_helpers.c
> b/arch/powerpc/platforms/pseries/of_helpers.c
> index 0185e50..11c90cd 100644
> --- a/arch/powerpc/platforms/pseries/of_helpers.c
> +++ b/arch/powerpc/platforms/pseries/of_helpers.c
> @@ -1,5 +1,7 @@
> // SPDX-License-Identifier: GPL-2.0
>
> +#define pr_fmt(fmt) "drc: " fmt
> +
> #include
> #include
> #include
> @@ -11,6 +13,12 @@
>
> #define MAX_DRC_NAME_LEN 64
>
> +static int drc_debug;
> +#define dbg(args...) if (drc_debug) { printk(KERN_DEBUG args); }
> +#define err(arg...) printk(KERN_ERR args);
> +#define info(arg...) printk(KERN_INFO args);
> +#define warn(arg...) printk(KERN_WARNING args);
Its pretty standard these days to use the pr_debug, pr_err, pr_info, pr_warn
variations over printk(LEVEL args).
> +
> /**
> * pseries_of_derive_parent - basically like dirname(1)
> * @path: the full_name of a node to be added to the tree
> @@ -46,7 +54,8 @@ struct device_node *pseries_of_derive_parent(const char
> *path)
>
> /* Helper Routines to convert between drc_index to cpu numbers */
>
> -int of_read_drc_info_cell(struct property **prop, const __be32 **curval,
> +static int of_read_drc_info_cell(struct property **prop,
> + const __be32 **curval,
> struct of_drc_info *data)
> {
> const char *p;
> @@ -90,4 +99,290 @@ int of_read_drc_info_cell(struct property **prop, const
> __be32 **curval,
>
> return 0;
> }
> -EXPORT_SYMBOL(of_read_drc_info_cell);
> +
> +static int walk_drc_info(struct device_node *dn,
> + bool (*usercb)(struct of_drc_info *drc,
> + void *data,
> + int *ret_code),
> + char *opt_drc_type,
> + void *data)
> +{
> + struct property *info;
> + unsigned int entries;
> + struct of_drc_info drc;
> + const __be32 *value;
> + int j, ret_code = -EINVAL;
> + bool done = false;
> +
> + info = of_find_property(dn, "ibm,drc-info", NULL);
> + if (info == NULL)
> + return -EINVAL;
> +
> + value = info->value;
> + entries = of_read_number(value++, 1);
> +
> + for (j = 0, done = 0; (j < entries) && (!done); j++) {
> + of_read_drc_info_cell(&info, &value, &drc);
> +
> + if (opt_drc_type && strcmp(opt_drc_type, drc.drc_type))
> + continue;
> +
> + done = usercb(&drc, data, &ret_code);
> + }
> +
> + return ret_code;
> +}
> +
> +static int get_children_props(struct device_node *dn, const int
> **drc_indexes,
> + const int **drc_names, const int **drc_types,
> + const int **drc_power_domains)
> +{
> + const int *indexes, *names, *types, *domains;
> +
> + indexes = of_get_property(dn, "ibm,drc-indexes", NULL);
> + names = of_get_property(dn, "ibm,drc-names", NULL);
> + types = of_get_property(dn, "ibm,drc-types", NULL);
> + domains = of_get_property(dn, "ibm,drc-power-domains", NULL);
> +
> + if (!indexes || !names || !types || !domains) {
> + /* Slot does not have dynamically-removable children */
> + return -EINVAL;
> + }
> + if (drc_indexes)
> + *drc_indexes = indexes;
> + if (drc_names)
> + /* &drc_names[1] contains NULL terminated slot names */
> + *drc_names = names;
> + if (drc_types)
> + /* &drc_types[1] contains NULL terminated slot types */
> + *drc_types = types;
> + if (drc_power_domains)
> + *drc_power_domains = domains;
> +
> + return 0;
> +}
> +
> +static int is_php_type(char *drc_ty
Re: [RFC 1/6] powerpc:/drc Define interface to acquire arch-specific drc info
On 12/14/2018 12:50 PM, Michael Bringmann wrote: > Define interface to acquire arch-specific drc info to match against > hotpluggable devices. The current implementation exposes several > pseries-specific dynamic memory properties in generic kernel code. > This patch set provides an interface to pull that code out of the > generic kernel. > > Signed-off-by: Michael Bringmann > --- > include/linux/topology.h |9 + > 1 file changed, 9 insertions(+) > > diff --git a/include/linux/topology.h b/include/linux/topology.h > index cb0775e..df97f5f 100644 > --- a/include/linux/topology.h > +++ b/include/linux/topology.h > @@ -44,6 +44,15 @@ As far as I know pseries is the only platform that uses DR connectors, and I highly doubt that any other powerpc platform or arch ever will. So, I'm not sure that this is really generic enough to belong in topology.h. If anything I would suggest putting this in an include in arch/powerpc/include/ named something like drcinfo.h or pseries-drc.h. That will make it visible to modules like rpaphp that want/need to use this functionality. -Tyrel > > int arch_update_cpu_topology(void); > > +int arch_find_drc_match(struct device_node *dn, > + bool (*usercb)(struct device_node *dn, > + u32 drc_index, char *drc_name, > + char *drc_type, u32 drc_power_domain, > + void *data), > + char *opt_drc_type, char *opt_drc_name, > + bool match_drc_index, bool ck_php_type, > + void *data); > + > /* Conform to ACPI 2.0 SLIT distance definitions */ > #define LOCAL_DISTANCE 10 > #define REMOTE_DISTANCE 20 >
Re: [PATCH] ibmvscsi: use GFP_ATOMIC with dma_alloc_coherent in map_sg_data
On 01/10/2019 07:07 AM, Christoph Hellwig wrote: > On Wed, Jan 09, 2019 at 06:58:56PM -0800, Tyrel Datwyler wrote: >> While mapping DMA for scatter list when a scsi command is queued the >> existing call to dma_alloc_coherent() in our map_sg_data() function >> passes zero for the gfp_flags parameter. We are most definitly in atomic >> context at this point as queue_command() is called in softirq context >> and further we have a spinlock holding the scsi host lock. >> >> Fix this by passing GFP_ATOMIC to dma_alloc_coherent() to prevent any >> sort of sleeping in atomic context deadlock. > > This is a pretty clear sign you should not be using dma_alloc_coherent > to start with. GFP_ATOMIC support in many of the implementations either > doesn't work at all or is severly constrained. On a secondary note I was unaware of the GFP_ATOMIC limitations. Should this be added to the documentation somewhere? I don't see any mention here form DMA-API-HOWTO.txt. Using Consistent DMA mappings = To allocate and map large (PAGE_SIZE or so) consistent DMA regions, you should do:: dma_addr_t dma_handle; cpu_addr = dma_alloc_coherent(dev, size, &dma_handle, gfp); where device is a ``struct device *``. This may be called in interrupt context with the GFP_ATOMIC flag. -Tyrel Given that the > descriptor is written by the OS and read by the hardware exactly once > there is no point in having the coherent mapping to start with. >
Re: Kconfig label updates
On 01/09/2019 04:37 AM, Michael Ellerman wrote: > Hi Bjorn, > > Bjorn Helgaas writes: >> Hi, >> >> I want to update the PCI Kconfig labels so they're more consistent and >> useful to users, something like the patch below. IIUC, the items >> below are all IBM-related; please correct me if not. >> >> I'd also like to expand (or remove) "RPA" because Google doesn't find >> anything about "IBM RPA", except Robotic Process Automation, which I >> think must be something else. > > Yeah I think just remove it, it's not a well known term and is unlikely > to help anyone these days. > > It stands for "RISC Platform Architecture", which was some kind of > specification for Power machines back in the day, but from what I can > tell it was never used in marketing or manuals much (hence so few hits > on Google). It is basically the predecessor to PAPR "Power Architecture Platform Reference". Which the LoPAPR document is available through power.org. Not sure if there is any desire to adopt PAPR in place of RPA. It is the platform reference doc that outlines how we do DLPAR and PCI Hotplug. > >> Is there some text expansion of RPA that we could use that would be >> meaningful to a user, i.e., something he/she might find on a nameplate >> or in a user manual? > > No I don't think so. > >> Ideally the PCI Kconfig labels would match the terms used in >> arch/.../Kconfig, e.g., >> >> config PPC_POWERNV >> bool "IBM PowerNV (Non-Virtualized) platform support" >> >> config PPC_PSERIES >> bool "IBM pSeries & new (POWER5-based) iSeries" > > TBH these are pretty unhelpful too. PowerNV is not a marketing name and > so doesn't appear anywhere much in official manuals or brochures and > it's also used on non-IBM branded machines. And pSeries & iSeries were > marketing names but are no longer used. pseries is still used as a machine type for PAPR compliant qemu/kvm instances. Again, just my 2 cents. I'm pretty open to whatever makes the most sense. -Tyrel > > We should probably update that text, but we can do that later, rather > than blocking this patch. > >> diff --git a/drivers/pci/hotplug/Kconfig b/drivers/pci/hotplug/Kconfig >> index e9f78eb390d2..1c1d145bfd84 100644 >> --- a/drivers/pci/hotplug/Kconfig >> +++ b/drivers/pci/hotplug/Kconfig >> @@ -112,7 +112,7 @@ config HOTPLUG_PCI_SHPC >>When in doubt, say N. >> >> config HOTPLUG_PCI_POWERNV >> -tristate "PowerPC PowerNV PCI Hotplug driver" >> +tristate "IBM PowerNV PCI Hotplug driver" > > This is used in non-IBM machines as well. > > So perhaps: ? > > tristate "IBM/OpenPower PowerNV (bare metal) PCI Hotplug driver" > >> @@ -125,10 +125,11 @@ config HOTPLUG_PCI_POWERNV >>When in doubt, say N. >> >> config HOTPLUG_PCI_RPA >> -tristate "RPA PCI Hotplug driver" >> +tristate "IBM Power Systems RPA PCI Hotplug driver" > > I think just drop RPA here. > >> depends on PPC_PSERIES && EEH >> help >>Say Y here if you have a RPA system that supports PCI Hotplug. > > s/RPA/IBM Power Systems/ > >> + This includes the earlier pSeries and iSeries. > > To be complete: > This includes the earlier System p, System i, pSeries and iSeries. > >> >>To compile this driver as a module, choose M here: the >>module will be called rpaphp. >> @@ -136,7 +137,7 @@ config HOTPLUG_PCI_RPA >>When in doubt, say N. >> >> config HOTPLUG_PCI_RPA_DLPAR >> -tristate "RPA Dynamic Logical Partitioning for I/O slots" >> +tristate "IBM RPA Dynamic Logical Partitioning for I/O slots" > > Again just drop RPA. > > > cheers >
Re: [PATCH] ibmvscsi: use GFP_ATOMIC with dma_alloc_coherent in map_sg_data
On 01/10/2019 07:07 AM, Christoph Hellwig wrote: > On Wed, Jan 09, 2019 at 06:58:56PM -0800, Tyrel Datwyler wrote: >> While mapping DMA for scatter list when a scsi command is queued the >> existing call to dma_alloc_coherent() in our map_sg_data() function >> passes zero for the gfp_flags parameter. We are most definitly in atomic >> context at this point as queue_command() is called in softirq context >> and further we have a spinlock holding the scsi host lock. >> >> Fix this by passing GFP_ATOMIC to dma_alloc_coherent() to prevent any >> sort of sleeping in atomic context deadlock. > > This is a pretty clear sign you should not be using dma_alloc_coherent > to start with. GFP_ATOMIC support in many of the implementations either > doesn't work at all or is severly constrained. Given that the > descriptor is written by the OS and read by the hardware exactly once > there is no point in having the coherent mapping to start with. > This allocation isn't a single use allocation. The driver is just lazy about allocating our ext_list area for large SG lists (ie. SG_ALL). When the driver was first written it only supported up to 10 indirect SRP buffers. James Bottemley added the large SG support back in 2005 with the commit referenced here in the fixes tag "4dddbc26c389". We only allocate the ext_list when we come across a SG list requiring more than 10 indirect buffers. Once allocated we will reuse if already allocated. -Tyrel
[PATCH] ibmvscsi: use GFP_KERNEL with dma_alloc_coherent in initialize_event_pool
During driver probe we allocate a dma region for our event pool.
Currently, zero is passed for the gfp_flags parameter. Driver probe
callbacks run in process context and we hold no locks so we can sleep
here if necessary.
Fix by passing GFP_KERNEL explicitly to dma_alloc_coherent().
Signed-off-by: Tyrel Datwyler
---
drivers/scsi/ibmvscsi/ibmvscsi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
index cb8535e..10d5e77 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.c
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
@@ -465,7 +465,7 @@ static int initialize_event_pool(struct event_pool *pool,
pool->iu_storage =
dma_alloc_coherent(hostdata->dev,
pool->size * sizeof(*pool->iu_storage),
- &pool->iu_token, 0);
+ &pool->iu_token, GFP_KERNEL);
if (!pool->iu_storage) {
kfree(pool->events);
return -ENOMEM;
--
1.8.3.1
[PATCH] ibmvscsi: use GFP_ATOMIC with dma_alloc_coherent in map_sg_data
While mapping DMA for scatter list when a scsi command is queued the
existing call to dma_alloc_coherent() in our map_sg_data() function
passes zero for the gfp_flags parameter. We are most definitly in atomic
context at this point as queue_command() is called in softirq context
and further we have a spinlock holding the scsi host lock.
Fix this by passing GFP_ATOMIC to dma_alloc_coherent() to prevent any
sort of sleeping in atomic context deadlock.
Fixes: 4dddbc26c389 ("[SCSI] ibmvscsi: handle large scatter/gather lists")
Cc: sta...@vger.kernel.org
Signed-off-by: Tyrel Datwyler
---
drivers/scsi/ibmvscsi/ibmvscsi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
index 1135e74..cb8535e 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.c
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
@@ -731,7 +731,7 @@ static int map_sg_data(struct scsi_cmnd *cmd,
evt_struct->ext_list = (struct srp_direct_buf *)
dma_alloc_coherent(dev,
SG_ALL * sizeof(struct
srp_direct_buf),
- &evt_struct->ext_list_token, 0);
+ &evt_struct->ext_list_token,
GFP_ATOMIC);
if (!evt_struct->ext_list) {
if (!firmware_has_feature(FW_FEATURE_CMO))
sdev_printk(KERN_ERR, cmd->device,
--
1.8.3.1
