* [PATCH] PCI: move early dump functionality from x86 arch into the common code
From: Sinan Kaya @ 2018-05-30 4:34 UTC (permalink / raw)
To: linux-pci, timur
Cc: linux-arm-msm, linux-arm-kernel, Sinan Kaya, Jonathan Corbet,
Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT), Bjorn Helgaas,
Christoffer Dall, Paul E. McKenney, Marc Zyngier, Kai-Heng Feng,
Thymo van Beers, Frederic Weisbecker, Konrad Rzeszutek Wilk,
Greg Kroah-Hartman, David Rientjes, Kate Stewart,
Philippe Ombredanne, Tom Lendacky, Juergen Gross, Borislav Petkov,
Mikulas Patocka, Petr Tesarik, Andy Lutomirski, Dou Liyang,
Ram Pai, Boris Ostrovsky, open list:DOCUMENTATION, open list
Move early dump functionality into common code so that it is available for
all archtiectures. No need to carry arch specific reads around as the read
hooks are already initialized by the time pci_setup_device() is getting
called during scan.
Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
Documentation/admin-guide/kernel-parameters.txt | 2 +-
arch/x86/include/asm/pci-direct.h | 5 ---
arch/x86/kernel/setup.c | 5 ---
arch/x86/pci/common.c | 4 --
arch/x86/pci/early.c | 50 -------------------------
drivers/pci/pci.c | 4 ++
drivers/pci/pci.h | 2 +-
drivers/pci/probe.c | 19 ++++++++++
8 files changed, 25 insertions(+), 66 deletions(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index c247612..4459270 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -2986,7 +2986,7 @@
See also Documentation/blockdev/paride.txt.
pci=option[,option...] [PCI] various PCI subsystem options:
- earlydump [X86] dump PCI config space before the kernel
+ earlydump dump PCI config space before the kernel
changes anything
off [X86] don't probe for the PCI bus
bios [X86-32] force use of PCI BIOS, don't access
diff --git a/arch/x86/include/asm/pci-direct.h b/arch/x86/include/asm/pci-direct.h
index e1084f7..e5e2129 100644
--- a/arch/x86/include/asm/pci-direct.h
+++ b/arch/x86/include/asm/pci-direct.h
@@ -14,9 +14,4 @@ extern void write_pci_config(u8 bus, u8 slot, u8 func, u8 offset, u32 val);
extern void write_pci_config_byte(u8 bus, u8 slot, u8 func, u8 offset, u8 val);
extern void write_pci_config_16(u8 bus, u8 slot, u8 func, u8 offset, u16 val);
-extern int early_pci_allowed(void);
-
-extern unsigned int pci_early_dump_regs;
-extern void early_dump_pci_device(u8 bus, u8 slot, u8 func);
-extern void early_dump_pci_devices(void);
#endif /* _ASM_X86_PCI_DIRECT_H */
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 2f86d88..480f250 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -991,11 +991,6 @@ void __init setup_arch(char **cmdline_p)
setup_clear_cpu_cap(X86_FEATURE_APIC);
}
-#ifdef CONFIG_PCI
- if (pci_early_dump_regs)
- early_dump_pci_devices();
-#endif
-
e820__reserve_setup_data();
e820__finish_early_params();
diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
index 563049c..d4ec117 100644
--- a/arch/x86/pci/common.c
+++ b/arch/x86/pci/common.c
@@ -22,7 +22,6 @@
unsigned int pci_probe = PCI_PROBE_BIOS | PCI_PROBE_CONF1 | PCI_PROBE_CONF2 |
PCI_PROBE_MMCONF;
-unsigned int pci_early_dump_regs;
static int pci_bf_sort;
int pci_routeirq;
int noioapicquirk;
@@ -599,9 +598,6 @@ char *__init pcibios_setup(char *str)
pci_probe |= PCI_BIG_ROOT_WINDOW;
return NULL;
#endif
- } else if (!strcmp(str, "earlydump")) {
- pci_early_dump_regs = 1;
- return NULL;
} else if (!strcmp(str, "routeirq")) {
pci_routeirq = 1;
return NULL;
diff --git a/arch/x86/pci/early.c b/arch/x86/pci/early.c
index e5f753c..e20d449 100644
--- a/arch/x86/pci/early.c
+++ b/arch/x86/pci/early.c
@@ -51,53 +51,3 @@ void write_pci_config_16(u8 bus, u8 slot, u8 func, u8 offset, u16 val)
outw(val, 0xcfc + (offset&2));
}
-int early_pci_allowed(void)
-{
- return (pci_probe & (PCI_PROBE_CONF1|PCI_PROBE_NOEARLY)) ==
- PCI_PROBE_CONF1;
-}
-
-void early_dump_pci_device(u8 bus, u8 slot, u8 func)
-{
- u32 value[256 / 4];
- int i;
-
- pr_info("pci 0000:%02x:%02x.%d config space:\n", bus, slot, func);
-
- for (i = 0; i < 256; i += 4)
- value[i / 4] = read_pci_config(bus, slot, func, i);
-
- print_hex_dump(KERN_INFO, "", DUMP_PREFIX_OFFSET, 16, 1, value, 256, false);
-}
-
-void early_dump_pci_devices(void)
-{
- unsigned bus, slot, func;
-
- if (!early_pci_allowed())
- return;
-
- for (bus = 0; bus < 256; bus++) {
- for (slot = 0; slot < 32; slot++) {
- for (func = 0; func < 8; func++) {
- u32 class;
- u8 type;
-
- class = read_pci_config(bus, slot, func,
- PCI_CLASS_REVISION);
- if (class == 0xffffffff)
- continue;
-
- early_dump_pci_device(bus, slot, func);
-
- if (func == 0) {
- type = read_pci_config_byte(bus, slot,
- func,
- PCI_HEADER_TYPE);
- if (!(type & 0x80))
- break;
- }
- }
- }
- }
-}
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 7c03701..ae5a2ae 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -115,6 +115,8 @@ static bool pcie_ari_disabled;
/* If set, the PCIe ATS capability will not be used. */
static bool pcie_ats_disabled;
+bool pci_early_dump;
+
bool pci_ats_disabled(void)
{
return pcie_ats_disabled;
@@ -5848,6 +5850,8 @@ static int __init pci_setup(char *str)
pcie_ats_disabled = true;
} else if (!strcmp(str, "noaer")) {
pci_no_aer();
+ } else if (!strcmp(str, "earlydump")) {
+ pci_early_dump = true;
} else if (!strncmp(str, "realloc=", 8)) {
pci_realloc_get_opt(str + 8);
} else if (!strncmp(str, "realloc", 7)) {
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index c358e7a0..9c66b7d 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -7,7 +7,7 @@
#define PCI_VSEC_ID_INTEL_TBT 0x1234 /* Thunderbolt */
extern const unsigned char pcie_link_speed[];
-
+extern bool pci_early_dump;
bool pcie_cap_has_lnkctl(const struct pci_dev *dev);
/* Functions internal to the PCI core code */
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 3840207..b1f068d 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1549,6 +1549,22 @@ static int pci_intx_mask_broken(struct pci_dev *dev)
return 0;
}
+static void early_dump_pci_device(struct pci_dev *pdev)
+{
+ u32 value[256 / 4];
+ int i;
+
+ dev_info(&pdev->dev, "pci 0000:%02x:%02x.%d config space:\n",
+ pdev->bus->number, PCI_SLOT(pdev->devfn),
+ PCI_FUNC(pdev->devfn));
+
+ for (i = 0; i < 256; i += 4)
+ pci_read_config_dword(pdev, i, &value[i / 4]);
+
+ print_hex_dump(KERN_INFO, "", DUMP_PREFIX_OFFSET, 16, 1, value,
+ 256, false);
+}
+
/**
* pci_setup_device - Fill in class and map information of a device
* @dev: the device structure to fill
@@ -1598,6 +1614,9 @@ int pci_setup_device(struct pci_dev *dev)
pci_printk(KERN_DEBUG, dev, "[%04x:%04x] type %02x class %#08x\n",
dev->vendor, dev->device, dev->hdr_type, dev->class);
+ if (pci_early_dump)
+ early_dump_pci_device(dev);
+
/* Need to have dev->class ready */
dev->cfg_size = pci_cfg_space_size(dev);
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* Re: [PATCH] PCI: move early dump functionality from x86 arch into the common code
From: Sinan Kaya @ 2018-05-30 4:36 UTC (permalink / raw)
To: linux-pci, timur
Cc: linux-arm-msm, linux-arm-kernel, Jonathan Corbet, Thomas Gleixner,
Ingo Molnar, H. Peter Anvin,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT), Bjorn Helgaas,
Christoffer Dall, Paul E. McKenney, Marc Zyngier, Kai-Heng Feng,
Thymo van Beers, Frederic Weisbecker, Konrad Rzeszutek Wilk,
Greg Kroah-Hartman, David Rientjes, Kate Stewart,
Philippe Ombredanne, Tom Lendacky, Juergen Gross, Borislav Petkov,
Mikulas Patocka, Petr Tesarik, Andy Lutomirski, Dou Liyang,
Ram Pai, Boris Ostrovsky, open list:DOCUMENTATION, open list
In-Reply-To: <1527654876-26716-1-git-send-email-okaya@codeaurora.org>
On 5/29/2018 9:34 PM, Sinan Kaya wrote:
> -int early_pci_allowed(void)
> -{
> - return (pci_probe & (PCI_PROBE_CONF1|PCI_PROBE_NOEARLY)) ==
> - PCI_PROBE_CONF1;
> -}
I should have kept this. I'll wait for more feedback before posting the
next rev.
--
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] PCI: Add pci=safemode option
From: Sinan Kaya @ 2018-05-30 4:41 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linux-pci, timur, linux-arm-msm, linux-arm-kernel,
Jonathan Corbet, Bjorn Helgaas, Thomas Gleixner, Ingo Molnar,
Christoffer Dall, Paul E. McKenney, Marc Zyngier, Kai-Heng Feng,
Thymo van Beers, Frederic Weisbecker, Konrad Rzeszutek Wilk,
David Rientjes, Rafael J. Wysocki, Keith Busch, Dongdong Liu,
Frederick Lawler, Oza Pawandeep, Gabriele Paoloni,
open list:DOCUMENTATION, open list
In-Reply-To: <20180530043103.GA19734@kroah.com>
On 5/29/2018 9:31 PM, Greg Kroah-Hartman wrote:
> On Tue, May 29, 2018 at 11:19:41PM -0400, Sinan Kaya wrote:
>> Adding pci=safemode kernel command line parameter to turn off all PCI
>> Express service driver as well as all optional PCIe features such as LTR,
>> Extended tags, Relaxed Ordering etc.
>>
>> Also setting MPS configuration to PCIE_BUS_SAFE so that MPS and MRRS can be
>> reconfigured with by the kernel in case BIOS hands off a broken
>> configuration.
>
> Why not fix the BIOS? That's what sane platforms do :)
>
>>
>> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
>> ---
>> Documentation/admin-guide/kernel-parameters.txt | 2 ++
>> drivers/pci/pci.c | 7 +++++++
>> drivers/pci/pci.h | 2 ++
>> drivers/pci/pcie/portdrv_core.c | 2 +-
>> drivers/pci/probe.c | 6 ++++++
>> 5 files changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>> index 641ec9c..247adbb 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -3153,6 +3153,8 @@
>> noari do not use PCIe ARI.
>> noats [PCIE, Intel-IOMMU, AMD-IOMMU]
>> do not use PCIe ATS (and IOMMU device IOTLB).
>> + safemode turns of all optinal PCI features. Useful
>> + for bringup/troubleshooting.
>
> s/optinal/optional/ ?
sure.
>
> And you should explain what exactly in PCI is "optional". Who defines
> this and where is that list and what can go wrong if those options are
> not enabled?
Bjorn and I discussed the need for such a "safe" mode feature when you
want to bring up PCI for a platform. You want to turn off everything as
a starter and just stick to bare minimum.
I can add a few words describing them. The goal of this option is to keep
base PCI features with MSI only. Things like PME, AER, ASPM, Extended
Tags, LTR, Relaxed Ordering, SRIOV are all considered optional. safemode
is certainly not intended for production environments.
I can taint the kernel as a suggestion.
I defined minimum as just booting a device and to be able to do DMA traffic
only with 0 optimization
>
> In looking at your patch, I can't determine that at all, so there's no
> way that someone just looking at this sentence will be able to
> understand.
>
> thanks,
>
> greg k-h
>
--
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] PCI: Add pci=safemode option
From: Greg Kroah-Hartman @ 2018-05-30 4:55 UTC (permalink / raw)
To: Sinan Kaya
Cc: linux-pci, timur, linux-arm-msm, linux-arm-kernel,
Jonathan Corbet, Bjorn Helgaas, Thomas Gleixner, Ingo Molnar,
Christoffer Dall, Paul E. McKenney, Marc Zyngier, Kai-Heng Feng,
Thymo van Beers, Frederic Weisbecker, Konrad Rzeszutek Wilk,
David Rientjes, Rafael J. Wysocki, Keith Busch, Dongdong Liu,
Frederick Lawler, Oza Pawandeep, Gabriele Paoloni,
open list:DOCUMENTATION, open list
In-Reply-To: <6c317ed8-cca3-8862-5f3b-12cf14e4d53b@codeaurora.org>
On Tue, May 29, 2018 at 09:41:33PM -0700, Sinan Kaya wrote:
> On 5/29/2018 9:31 PM, Greg Kroah-Hartman wrote:
> > On Tue, May 29, 2018 at 11:19:41PM -0400, Sinan Kaya wrote:
> >> Adding pci=safemode kernel command line parameter to turn off all PCI
> >> Express service driver as well as all optional PCIe features such as LTR,
> >> Extended tags, Relaxed Ordering etc.
> >>
> >> Also setting MPS configuration to PCIE_BUS_SAFE so that MPS and MRRS can be
> >> reconfigured with by the kernel in case BIOS hands off a broken
> >> configuration.
> >
> > Why not fix the BIOS? That's what sane platforms do :)
> >
> >>
> >> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> >> ---
> >> Documentation/admin-guide/kernel-parameters.txt | 2 ++
> >> drivers/pci/pci.c | 7 +++++++
> >> drivers/pci/pci.h | 2 ++
> >> drivers/pci/pcie/portdrv_core.c | 2 +-
> >> drivers/pci/probe.c | 6 ++++++
> >> 5 files changed, 18 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> >> index 641ec9c..247adbb 100644
> >> --- a/Documentation/admin-guide/kernel-parameters.txt
> >> +++ b/Documentation/admin-guide/kernel-parameters.txt
> >> @@ -3153,6 +3153,8 @@
> >> noari do not use PCIe ARI.
> >> noats [PCIE, Intel-IOMMU, AMD-IOMMU]
> >> do not use PCIe ATS (and IOMMU device IOTLB).
> >> + safemode turns of all optinal PCI features. Useful
> >> + for bringup/troubleshooting.
> >
> > s/optinal/optional/ ?
>
> sure.
>
> >
> > And you should explain what exactly in PCI is "optional". Who defines
> > this and where is that list and what can go wrong if those options are
> > not enabled?
>
> Bjorn and I discussed the need for such a "safe" mode feature when you
> want to bring up PCI for a platform. You want to turn off everything as
> a starter and just stick to bare minimum.
>
> I can add a few words describing them. The goal of this option is to keep
> base PCI features with MSI only. Things like PME, AER, ASPM, Extended
> Tags, LTR, Relaxed Ordering, SRIOV are all considered optional. safemode
> is certainly not intended for production environments.
Ok, then you should say that here, or somewhere, so that people know
this. Otherwise people will see that "hey this lets my hardware boot!"
and then never change it :(
> I can taint the kernel as a suggestion.
I would not worry about that.
> I defined minimum as just booting a device and to be able to do DMA traffic
> only with 0 optimization
Ok, again, just document this really well, so that people do not have
questions and start wondering why their devices barely seem to work.
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] PCI: Add pci=safemode option
From: Christoph Hellwig @ 2018-05-30 7:37 UTC (permalink / raw)
To: Sinan Kaya
Cc: Greg Kroah-Hartman, linux-pci, timur, linux-arm-msm,
linux-arm-kernel, Jonathan Corbet, Bjorn Helgaas, Thomas Gleixner,
Ingo Molnar, Christoffer Dall, Paul E. McKenney, Marc Zyngier,
Kai-Heng Feng, Thymo van Beers, Frederic Weisbecker,
Konrad Rzeszutek Wilk, David Rientjes, Rafael J. Wysocki,
Keith Busch, Dongdong Liu, Frederick Lawler, Oza Pawandeep,
Gabriele Paoloni, open list:DOCUMENTATION, open list
In-Reply-To: <6c317ed8-cca3-8862-5f3b-12cf14e4d53b@codeaurora.org>
On Tue, May 29, 2018 at 09:41:33PM -0700, Sinan Kaya wrote:
> Bjorn and I discussed the need for such a "safe" mode feature when you
> want to bring up PCI for a platform. You want to turn off everything as
> a starter and just stick to bare minimum.
Can we please make it a config option the instead of adding code
to every kernel? Also maybe the bringup should be in the name
to make this more clear?
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] PCI: Add pci=safemode option
From: okaya @ 2018-05-30 7:44 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Greg Kroah-Hartman, linux-pci, timur, linux-arm-msm,
linux-arm-kernel, Jonathan Corbet, Bjorn Helgaas, Thomas Gleixner,
Ingo Molnar, Christoffer Dall, Paul E. McKenney, Marc Zyngier,
Kai-Heng Feng, Thymo van Beers, Frederic Weisbecker,
Konrad Rzeszutek Wilk, David Rientjes, Rafael J. Wysocki,
Keith Busch, Dongdong Liu, Frederick Lawler, Oza Pawandeep,
Gabriele Paoloni, open list:DOCUMENTATION, open list
In-Reply-To: <20180530073735.GA28793@infradead.org>
On 2018-05-30 00:37, Christoph Hellwig wrote:
> On Tue, May 29, 2018 at 09:41:33PM -0700, Sinan Kaya wrote:
>> Bjorn and I discussed the need for such a "safe" mode feature when you
>> want to bring up PCI for a platform. You want to turn off everything
>> as
>> a starter and just stick to bare minimum.
>
> Can we please make it a config option the instead of adding code
> to every kernel? Also maybe the bringup should be in the name
> to make this more clear?
One other requirement was to have a runtime option rather than compile
time option.
When someone reported a problem, we wanted to be able to say "use this
option and see if system boots" without doing any bisects or
recompilation.
This would be the first step in troubleshooting a system to see if
fundamental features are working.
I don't mind changing the name
Bjorn mentioned safe option. I made it safemode. I am looking at Bjorn
for suggestions at this moment.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] PCI: Add pci=safemode option
From: Greg Kroah-Hartman @ 2018-05-30 7:48 UTC (permalink / raw)
To: okaya
Cc: Christoph Hellwig, linux-pci, timur, linux-arm-msm,
linux-arm-kernel, Jonathan Corbet, Bjorn Helgaas, Thomas Gleixner,
Ingo Molnar, Christoffer Dall, Paul E. McKenney, Marc Zyngier,
Kai-Heng Feng, Thymo van Beers, Frederic Weisbecker,
Konrad Rzeszutek Wilk, David Rientjes, Rafael J. Wysocki,
Keith Busch, Dongdong Liu, Frederick Lawler, Oza Pawandeep,
Gabriele Paoloni, open list:DOCUMENTATION, open list
In-Reply-To: <6dfe2db8f974d94c9867f30ec83d9333@codeaurora.org>
On Wed, May 30, 2018 at 12:44:29AM -0700, okaya@codeaurora.org wrote:
> On 2018-05-30 00:37, Christoph Hellwig wrote:
> > On Tue, May 29, 2018 at 09:41:33PM -0700, Sinan Kaya wrote:
> > > Bjorn and I discussed the need for such a "safe" mode feature when you
> > > want to bring up PCI for a platform. You want to turn off everything
> > > as
> > > a starter and just stick to bare minimum.
> >
> > Can we please make it a config option the instead of adding code
> > to every kernel? Also maybe the bringup should be in the name
> > to make this more clear?
>
> One other requirement was to have a runtime option rather than compile time
> option.
>
> When someone reported a problem, we wanted to be able to say "use this
> option and see if system boots" without doing any bisects or recompilation.
>
> This would be the first step in troubleshooting a system to see if
> fundamental features are working.
That makes sense, people can not rebuild their kernels for the most
part. Putting it behind a config option would not make sense as it
would always have to be enabled.
> I don't mind changing the name Bjorn mentioned safe option. I made it
> safemode. I am looking at Bjorn for suggestions at this moment.
"minimal"? "basic"? "crippled"?
"my_hardware_is_so_borked_it_needs_this_option"? :)
Naming is hard...
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] PCI: Add pci=safemode option
From: okaya @ 2018-05-30 7:56 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Christoph Hellwig, linux-pci, timur, linux-arm-msm,
linux-arm-kernel, Jonathan Corbet, Bjorn Helgaas, Thomas Gleixner,
Ingo Molnar, Christoffer Dall, Paul E. McKenney, Marc Zyngier,
Kai-Heng Feng, Thymo van Beers, Frederic Weisbecker,
Konrad Rzeszutek Wilk, David Rientjes, Rafael J. Wysocki,
Keith Busch, Dongdong Liu, Frederick Lawler, Oza Pawandeep,
Gabriele Paoloni, open list:DOCUMENTATION, open list,
linux-pci-owner
In-Reply-To: <20180530074822.GB30177@kroah.com>
On 2018-05-30 00:48, Greg Kroah-Hartman wrote:
> On Wed, May 30, 2018 at 12:44:29AM -0700, okaya@codeaurora.org wrote:
>> On 2018-05-30 00:37, Christoph Hellwig wrote:
>> > On Tue, May 29, 2018 at 09:41:33PM -0700, Sinan Kaya wrote:
>> > > Bjorn and I discussed the need for such a "safe" mode feature when you
>> > > want to bring up PCI for a platform. You want to turn off everything
>> > > as
>> > > a starter and just stick to bare minimum.
>> >
>> > Can we please make it a config option the instead of adding code
>> > to every kernel? Also maybe the bringup should be in the name
>> > to make this more clear?
>>
>> One other requirement was to have a runtime option rather than compile
>> time
>> option.
>>
>> When someone reported a problem, we wanted to be able to say "use this
>> option and see if system boots" without doing any bisects or
>> recompilation.
>>
>> This would be the first step in troubleshooting a system to see if
>> fundamental features are working.
>
> That makes sense, people can not rebuild their kernels for the most
> part. Putting it behind a config option would not make sense as it
> would always have to be enabled.
>
Here is where the discussion took place. Last 5-10 messages should
help.
https://bugzilla.kernel.org/show_bug.cgi?id=196197
>> I don't mind changing the name Bjorn mentioned safe option. I made it
>> safemode. I am looking at Bjorn for suggestions at this moment.
>
> "minimal"? "basic"? "crippled"?
> "my_hardware_is_so_borked_it_needs_this_option"? :)
>
> Naming is hard...
>
> greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] PCI: Add pci=safemode option
From: okaya @ 2018-05-30 8:22 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Christoph Hellwig, linux-pci, timur, linux-arm-msm,
linux-arm-kernel, Jonathan Corbet, Bjorn Helgaas, Thomas Gleixner,
Ingo Molnar, Christoffer Dall, Paul E. McKenney, Marc Zyngier,
Kai-Heng Feng, Thymo van Beers, Frederic Weisbecker,
Konrad Rzeszutek Wilk, David Rientjes, Rafael J. Wysocki,
Keith Busch, Dongdong Liu, Frederick Lawler, Oza Pawandeep,
Gabriele Paoloni, open list:DOCUMENTATION, open list,
linux-pci-owner
In-Reply-To: <577f01ada5e7f08c79a28d41020fb019@codeaurora.org>
On 2018-05-30 00:56, okaya@codeaurora.org wrote:
> On 2018-05-30 00:48, Greg Kroah-Hartman wrote:
>> On Wed, May 30, 2018 at 12:44:29AM -0700, okaya@codeaurora.org wrote:
>>> On 2018-05-30 00:37, Christoph Hellwig wrote:
>>> > On Tue, May 29, 2018 at 09:41:33PM -0700, Sinan Kaya wrote:
>>> > > Bjorn and I discussed the need for such a "safe" mode feature when you
>>> > > want to bring up PCI for a platform. You want to turn off everything
>>> > > as
>>> > > a starter and just stick to bare minimum.
>>> >
>>> > Can we please make it a config option the instead of adding code
>>> > to every kernel? Also maybe the bringup should be in the name
>>> > to make this more clear?
>>>
>>> One other requirement was to have a runtime option rather than
>>> compile time
>>> option.
>>>
>>> When someone reported a problem, we wanted to be able to say "use
>>> this
>>> option and see if system boots" without doing any bisects or
>>> recompilation.
>>>
>>> This would be the first step in troubleshooting a system to see if
>>> fundamental features are working.
>>
>> That makes sense, people can not rebuild their kernels for the most
>> part. Putting it behind a config option would not make sense as it
>> would always have to be enabled.
>>
>
> Here is where the discussion took place. Last 5-10 messages should
> help.
>
>
> https://bugzilla.kernel.org/show_bug.cgi?id=196197
>
Some more paper trail for general awareness.
https://lkml.org/lkml/2018/5/3/509
>
>>> I don't mind changing the name Bjorn mentioned safe option. I made it
>>> safemode. I am looking at Bjorn for suggestions at this moment.
>>
>> "minimal"? "basic"? "crippled"?
>> "my_hardware_is_so_borked_it_needs_this_option"? :)
>>
>> Naming is hard...
>>
>> greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [RFT v3 1/4] perf cs-etm: Generate branch sample for missed packets
From: Robert Walker @ 2018-05-30 9:45 UTC (permalink / raw)
To: Mathieu Poirier, Leo Yan
Cc: Arnaldo Carvalho de Melo, Jonathan Corbet, mike.leach,
kim.phillips, Tor Jeremiassen, Peter Zijlstra, Ingo Molnar,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, linux-arm-kernel,
linux-doc, linux-kernel, coresight
In-Reply-To: <20180528221347.GA4109@xps15>
On 28/05/18 23:13, Mathieu Poirier wrote:
> Leo and/or Robert,
>
> On Mon, May 28, 2018 at 04:45:00PM +0800, Leo Yan wrote:
>> Commit e573e978fb12 ("perf cs-etm: Inject capabilitity for CoreSight
>> traces") reworks the samples generation flow from CoreSight trace to
>> match the correct format so Perf report tool can display the samples
>> properly.
>>
>> But the change has side effect for branch packet handling, it only
>> generate branch samples by checking previous packet flag
>> 'last_instr_taken_branch' is true, this results in below three kinds
>> packets are missed to generate branch samples:
>>
>> - The start tracing packet at the beginning of tracing data;
>> - The exception handling packet;
>> - If one CS_ETM_TRACE_ON packet is inserted, we also miss to handle it
>> for branch samples. CS_ETM_TRACE_ON packet itself can give the info
>> that there have a discontinuity in the trace, on the other hand we
>> also miss to generate proper branch sample for packets before and
>> after CS_ETM_TRACE_ON packet.
>>
>> This patch is to add branch sample handling for up three kinds packets:
>>
>> - In function cs_etm__sample(), check if 'prev_packet->sample_type' is
>> zero and in this case it generates branch sample for the start tracing
>> packet; furthermore, we also need to handle the condition for
>> prev_packet::end_addr is zero in the cs_etm__last_executed_instr();
>>
>> - In function cs_etm__sample(), check if 'prev_packet->exc' is true and
>> generate branch sample for exception handling packet;
>>
>> - If there has one CS_ETM_TRACE_ON packet is coming, we firstly generate
>> branch sample in the function cs_etm__flush(), this can save complete
>> info for the previous CS_ETM_RANGE packet just before CS_ETM_TRACE_ON
>> packet. We also generate branch sample for the new CS_ETM_RANGE
>> packet after CS_ETM_TRACE_ON packet, this have two purposes, the
>> first one purpose is to save the info for the new CS_ETM_RANGE packet,
>> the second purpose is to save CS_ETM_TRACE_ON packet info so we can
>> have hint for a discontinuity in the trace.
>>
>> For CS_ETM_TRACE_ON packet, its fields 'packet->start_addr' and
>> 'packet->end_addr' equal to 0xdeadbeefdeadbeefUL which are emitted in
>> the decoder layer as dummy value. This patch is to convert these
>> values to zeros for more readable; this is accomplished by functions
>> cs_etm__last_executed_instr() and cs_etm__first_executed_instr(). The
>> later one is a new function introduced by this patch.
>>
>> Reviewed-by: Robert Walker <robert.walker@arm.com>
>> Fixes: e573e978fb12 ("perf cs-etm: Inject capabilitity for CoreSight traces")
>> Signed-off-by: Leo Yan <leo.yan@linaro.org>
>> ---
>> tools/perf/util/cs-etm.c | 93 +++++++++++++++++++++++++++++++++++++-----------
>> 1 file changed, 73 insertions(+), 20 deletions(-)
>>
>> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
>> index 822ba91..8418173 100644
>> --- a/tools/perf/util/cs-etm.c
>> +++ b/tools/perf/util/cs-etm.c
>> @@ -495,6 +495,20 @@ static inline void cs_etm__reset_last_branch_rb(struct cs_etm_queue *etmq)
>> static inline u64 cs_etm__last_executed_instr(struct cs_etm_packet *packet)
>> {
>> /*
>> + * The packet is the start tracing packet if the end_addr is zero,
>> + * returns 0 for this case.
>> + */
>> + if (!packet->end_addr)
>> + return 0;
>
> What is considered to be the "start tracing packet"? Right now the only two
> kind of packets inserted in the decoder packet buffer queue are INST_RANGE and
> TRACE_ON. How can we hit a condition where packet->end-addr == 0?
>
>
>> +
>> + /*
>> + * The packet is the CS_ETM_TRACE_ON packet if the end_addr is
>> + * magic number 0xdeadbeefdeadbeefUL, returns 0 for this case.
>> + */
>> + if (packet->end_addr == 0xdeadbeefdeadbeefUL)
>> + return 0;
>
> As it is with the above, I find triggering on addresses to be brittle and hard
> to maintain on the long run. Packets all have a sample_type field that should
> be used in cases like this one. That way we know exactly the condition that is
> targeted.
>
> While working on this set, please spin-off another patch that defines
> CS_ETM_INVAL_ADDR 0xdeadbeefdeadbeefUL and replace all the cases where the
> numeral is used. That way we stop using the hard coded value.
>
>> +
>> + /*
>> * The packet records the execution range with an exclusive end address
>> *
>> * A64 instructions are constant size, so the last executed
>> @@ -505,6 +519,18 @@ static inline u64 cs_etm__last_executed_instr(struct cs_etm_packet *packet)
>> return packet->end_addr - A64_INSTR_SIZE;
>> }
>>
>> +static inline u64 cs_etm__first_executed_instr(struct cs_etm_packet *packet)
>> +{
>> + /*
>> + * The packet is the CS_ETM_TRACE_ON packet if the start_addr is
>> + * magic number 0xdeadbeefdeadbeefUL, returns 0 for this case.
>> + */
>> + if (packet->start_addr == 0xdeadbeefdeadbeefUL)
>> + return 0;
>
> Same comment as above.
>
>> +
>> + return packet->start_addr;
>> +}
>> +
>> static inline u64 cs_etm__instr_count(const struct cs_etm_packet *packet)
>> {
>> /*
>> @@ -546,7 +572,7 @@ static void cs_etm__update_last_branch_rb(struct cs_etm_queue *etmq)
>>
>> be = &bs->entries[etmq->last_branch_pos];
>> be->from = cs_etm__last_executed_instr(etmq->prev_packet);
>> - be->to = etmq->packet->start_addr;
>> + be->to = cs_etm__first_executed_instr(etmq->packet);
>> /* No support for mispredict */
>> be->flags.mispred = 0;
>> be->flags.predicted = 1;
>> @@ -701,7 +727,7 @@ static int cs_etm__synth_branch_sample(struct cs_etm_queue *etmq)
>> sample.ip = cs_etm__last_executed_instr(etmq->prev_packet);
>> sample.pid = etmq->pid;
>> sample.tid = etmq->tid;
>> - sample.addr = etmq->packet->start_addr;
>> + sample.addr = cs_etm__first_executed_instr(etmq->packet);
>> sample.id = etmq->etm->branches_id;
>> sample.stream_id = etmq->etm->branches_id;
>> sample.period = 1;
>> @@ -897,13 +923,28 @@ static int cs_etm__sample(struct cs_etm_queue *etmq)
>> etmq->period_instructions = instrs_over;
>> }
>>
>> - if (etm->sample_branches &&
>> - etmq->prev_packet &&
>> - etmq->prev_packet->sample_type == CS_ETM_RANGE &&
>> - etmq->prev_packet->last_instr_taken_branch) {
>> - ret = cs_etm__synth_branch_sample(etmq);
>> - if (ret)
>> - return ret;
>> + if (etm->sample_branches && etmq->prev_packet) {
>> + bool generate_sample = false;
>> +
>> + /* Generate sample for start tracing packet */
>> + if (etmq->prev_packet->sample_type == 0 ||
>
> What kind of packet is sample_type == 0 ?
>
>> + etmq->prev_packet->sample_type == CS_ETM_TRACE_ON)
>> + generate_sample = true;
>> +
>> + /* Generate sample for exception packet */
>> + if (etmq->prev_packet->exc == true)
>> + generate_sample = true;
>
> Please don't do that. Exception packets have a type of their own and can be
> added to the decoder packet queue the same way INST_RANGE and TRACE_ON packets
> are. Moreover exception packet containt an address that, if I'm reading the
> documenation properly, can be used to keep track of instructions that were
> executed between the last address of the previous range packet and the address
> executed just before the exception occurred. Mike and Rob will have to confirm
> this as the decoder may be doing all that hard work for us.
>
>> +
>> + /* Generate sample for normal branch packet */
>> + if (etmq->prev_packet->sample_type == CS_ETM_RANGE &&
>> + etmq->prev_packet->last_instr_taken_branch)
>> + generate_sample = true;
>> +
>> + if (generate_sample) {
>> + ret = cs_etm__synth_branch_sample(etmq);
>> + if (ret)
>> + return ret;
>> + }
>> }
>>
>> if (etm->sample_branches || etm->synth_opts.last_branch) {
>> @@ -922,11 +963,16 @@ static int cs_etm__sample(struct cs_etm_queue *etmq)
>> static int cs_etm__flush(struct cs_etm_queue *etmq)
>> {
>> int err = 0;
>> + struct cs_etm_auxtrace *etm = etmq->etm;
>> struct cs_etm_packet *tmp;
>>
>> - if (etmq->etm->synth_opts.last_branch &&
>> - etmq->prev_packet &&
>> - etmq->prev_packet->sample_type == CS_ETM_RANGE) {
>> + if (!etmq->prev_packet)
>> + return 0;
>> +
>> + if (etmq->prev_packet->sample_type != CS_ETM_RANGE)
>> + return 0;
>> +
>> + if (etmq->etm->synth_opts.last_branch) {
>
> If you add:
>
> if (!etmq->etm->synth_opts.last_branch)
> return 0;
>
> You can avoid indenting the whole block.
>
>> /*
>> * Generate a last branch event for the branches left in the
>> * circular buffer at the end of the trace.
>> @@ -939,18 +985,25 @@ static int cs_etm__flush(struct cs_etm_queue *etmq)
>> err = cs_etm__synth_instruction_sample(
>> etmq, addr,
>> etmq->period_instructions);
>> + if (err)
>> + return err;
>> etmq->period_instructions = 0;
>> + }
>>
>> - /*
>> - * Swap PACKET with PREV_PACKET: PACKET becomes PREV_PACKET for
>> - * the next incoming packet.
>> - */
>> - tmp = etmq->packet;
>> - etmq->packet = etmq->prev_packet;
>> - etmq->prev_packet = tmp;
>> + if (etm->sample_branches) {
>> + err = cs_etm__synth_branch_sample(etmq);
>> + if (err)
>> + return err;
>> }
>>
>> - return err;
>> + /*
>> + * Swap PACKET with PREV_PACKET: PACKET becomes PREV_PACKET for
>> + * the next incoming packet.
>> + */
>> + tmp = etmq->packet;
>> + etmq->packet = etmq->prev_packet;
>> + etmq->prev_packet = tmp;
>
> Robert, I remember noticing that when you first submitted the code but forgot to
> go back to it. What is the point of swapping the packets? I understand
>
> etmq->prev_packet = etmq->packet;
>
> But not
>
> etmq->packet = tmp;
>
> After all etmq->packet will be clobbered as soon as cs_etm_decoder__get_packet()
> is called, which is alwasy right after either cs_etm__sample() or
> cs_etm__flush().
>
This is code I inherited from the original versions of these patches,
but it works because:
- etmq->packet and etmq->prev_packet are pointers to struct
cs_etm_packet allocated by zalloc() in cs_etm__alloc_queue()
- cs_etm_decoder__get_packet() takes a pointer to struct cs_etm_packet
and copies the contents of the first packet from the queue into the
passed location with:
*packet = decoder->packet_buffer[decoder->head]
So the swap code is only swapping the pointers over, not the contents of
the packets.
Regards
Rob
> Thanks,
> Mathieu
>
>
>
>> + return 0;
>> }
>>
>> static int cs_etm__run_decoder(struct cs_etm_queue *etmq)
>> --
>> 2.7.4
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v9 0/7] Enable cpuset controller in default hierarchy
From: Juri Lelli @ 2018-05-30 10:13 UTC (permalink / raw)
To: Waiman Long
Cc: Tejun Heo, Li Zefan, Johannes Weiner, Peter Zijlstra, Ingo Molnar,
cgroups, linux-kernel, linux-doc, kernel-team, pjt, luto,
Mike Galbraith, torvalds, Roman Gushchin, Patrick Bellasi
In-Reply-To: <1527601294-3444-1-git-send-email-longman@redhat.com>
Hi,
On 29/05/18 09:41, Waiman Long wrote:
> v9:
> - Rename cpuset.sched.domain to cpuset.sched.domain_root to better
> identify its purpose as the root of a new scheduling domain or
> partition.
> - Clarify in the document about the purpose of domain_root and
> load_balance. Using domain_root is th only way to create new
> partition.
> - Fix a lockdep warning in update_isolated_cpumask() function.
> - Add a new patch to eliminate call to generate_sched_domains() for
> v2 when a change in cpu list does not touch a domain_root.
I was playing with this and ended up with the situation below:
g1/cgroup.controllers:cpuset
g1/cgroup.events:populated 0
g1/cgroup.max.depth:max
g1/cgroup.max.descendants:max
g1/cgroup.stat:nr_descendants 1
g1/cgroup.stat:nr_dying_descendants 0
g1/cgroup.subtree_control:cpuset
g1/cgroup.type:domain
g1/cpuset.cpus:0-5 <---
g1/cpuset.cpus.effective:0-5
g1/cpuset.mems.effective:0-1
g1/cpuset.sched.domain_root:1 <---
g1/cpuset.sched.load_balance:1
g1/cpu.stat:usage_usec 0
g1/cpu.stat:user_usec 0
g1/cpu.stat:system_usec 0
g1/g11/cgroup.events:populated 0
g1/g11/cgroup.max.descendants:max
g1/g11/cpu.stat:usage_usec 0
g1/g11/cpu.stat:user_usec 0
g1/g11/cpu.stat:system_usec 0
g1/g11/cgroup.type:domain
g1/g11/cgroup.stat:nr_descendants 0
g1/g11/cgroup.stat:nr_dying_descendants 0
g1/g11/cpuset.cpus.effective:0-5
g1/g11/cgroup.controllers:cpuset
g1/g11/cpuset.sched.load_balance:1
g1/g11/cpuset.mems.effective:0-1
g1/g11/cpuset.cpus:6-11 <---
g1/g11/cgroup.max.depth:max
g1/g11/cpuset.sched.domain_root:0
Should this be allowed? I was expecting subgroup g11 should be
restricted to a subset of g1's cpus.
Best,
- Juri
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v3 1/3] usb: gadget: ccid: add support for USB CCID Gadget Device
From: Marcus Folkesson @ 2018-05-30 11:24 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Greg Kroah-Hartman, Jonathan Corbet, Felipe Balbi,
David S. Miller, Mauro Carvalho Chehab, Andrew Morton,
Randy Dunlap, Ruslan Bilovol, Thomas Gleixner, Kate Stewart, USB,
Linux Documentation List, Linux Kernel Mailing List
In-Reply-To: <CAHp75VeSbC3QBswCxZpoVpT4cL4pXC5eouCc9YtvBtXG3YddTg@mail.gmail.com>
Hi Andy,
Thank you for your comments!
Many good catches here!
On Wed, May 30, 2018 at 03:55:39AM +0300, Andy Shevchenko wrote:
> On Tue, May 29, 2018 at 9:50 PM, Marcus Folkesson
> <marcus.folkesson@gmail.com> wrote:
> > Chip Card Interface Device (CCID) protocol is a USB protocol that
> > allows a smartcard device to be connected to a computer via a card
> > reader using a standard USB interface, without the need for each manufacturer
> > of smartcards to provide its own reader or protocol.
> >
> > This gadget driver makes Linux show up as a CCID device to the host and let a
> > userspace daemon act as the smartcard.
> >
> > This is useful when the Linux gadget itself should act as a cryptographic
> > device or forward APDUs to an embedded smartcard device.
>
> > + * Copyright (C) 2018 Marcus Folkesson <marcus.folkesson@gmail.com>
>
> > + *
>
> Redundant line
>
Yep
> > +static DEFINE_IDA(ccidg_ida);
>
> Where is it destroyed?
Hm, I'm not sure it needs to be destroyed. From lib/idr.c:
* You can also use ida_get_new_above() if you need an ID to be allocated
* above a particular number. ida_destroy() can be used to dispose of an
* IDA without needing to free the individual IDs in it. You can use
* ida_is_empty() to find out whether the IDA has any IDs currently allocated.
An empty ccidg_ida is the indication that we should clean up our
mess:
static void ccidg_free_inst(struct usb_function_instance *f)
...
if (ida_is_empty(&ccidg_ida))
ccidg_cleanup();
If the IDA is empty, should I call ida_destroy() anyway?
Other similiar drivers does not seems to do that.
I must say that I'm not very familiar with the IDA API.
>
> > + ccidg_class = NULL;
> > + return PTR_ERR(ccidg_class);
>
> Are you sure?
Heh, :-)
>
> > + if (!list_empty(head)) {
> > + req = list_first_entry(head, struct usb_request, list);
>
> list_first_entry_or_null()
Will do, thanks.
>
> > + req->length = len;
>
> Perhaps assign this obly if malloc successedeed ?
Will do.
>
> > + req->buf = kmalloc(len, GFP_ATOMIC);
>
> > + if (req->buf == NULL) {
>
> if (!req->buf) ?
Will do
>
> > + usb_ep_free_request(ep, req);
> > + return ERR_PTR(-ENOMEM);
> > + }
>
> > +static void ccidg_request_free(struct usb_request *req, struct usb_ep *ep)
> > +{
>
> > + if (req) {
>
> Is it even possible?
>
> What about
>
> if (!req)
> return;
>
> ?
Hmm, maybe it is not.
I think I remove this check.
>
> > + kfree(req->buf);
> > + usb_ep_free_request(ep, req);
> > + }
> > +}
>
> > + *(__le32 *) req->buf = ccid_class_desc.dwDefaultClock;
>
> Hmm... put_unaligned()? cpu_to_le32()? cpu_to_le32p()?
>
Hmm, dwDefaultClock is already represented in little endian format. If I
use any of those functions, would not the byte order be swaped on a big
endian system?
dwDefaultClock is set as:
.dwDefaultClock = cpu_to_le32(3580),
I'm not sure what the best practice is here.
> > + *(__le32 *) req->buf = ccid_class_desc.dwDataRate;
>
> Ditto.
>
> > + }
> > + }
>
> Indentation.
I remove the extra brackets from the case instead.
>
> > + /* responded with data transfer or status phase? */
> > + if (ret >= 0) {
>
> Why not
>
> if (ret < 0)
> return ret;
>
> ?
>
Will do
> > + }
> > +
> > + return ret;
> > +}
>
> > + atomic_set(&ccidg->online, 1);
> > + return ret;
>
> return 0; ?
Will do
>
> > + struct f_ccidg *ccidg;
>
> > + ccidg = container_of(inode->i_cdev, struct f_ccidg, cdev);
>
> One line ?
The line exceeds 80 characters then, but I will put it like this:
struct f_ccidg *ccidg = container_of(inode->i_cdev,
struct f_ccidg, cdev);
>
> > + xfer = (req->actual < count) ? req->actual : count;
>
> min_t()
>
Thanks, will do
> > + ret = wait_event_interruptible(bulk_dev->write_wq,
> > + ((req = ccidg_req_get(ccidg, &bulk_dev->tx_idle))));
> > +
> > + if (ret < 0)
> > + return -ERESTARTSYS;
>
> Redundant blank line above.
>
I remove the extra blank line
> > +static void ccidg_function_free(struct usb_function *f)
> > +{
>
> > + struct f_ccidg_opts *opts;
>
> > + opts = container_of(f->fi, struct f_ccidg_opts, func_inst);
>
> One line.
See above
>
>
> > + mutex_lock(&opts->lock);
> > + --opts->refcnt;
>
> -- will work
Will do
>
> > + mutex_unlock(&opts->lock);
> > +}
>
> > + struct f_ccidg_opts *opts;
>
> > + opts = container_of(fi, struct f_ccidg_opts, func_inst);
>
> Perhaps one line ?
See above
> > + ++opts->refcnt;
> X++ would work as well.
Will do
> > + struct f_ccidg_opts *opts;
> > +
> > + opts = container_of(f, struct f_ccidg_opts, func_inst);
>
> Perhaps one line?
>
See above
> > +#define CCID_PINSUPOORT_NONE 0x00
>
> (0 << 0)
>
> ?
>
> for sake of consistency
Yep, will change
>
> > +#define CCID_PINSUPOORT_VERIFICATION (1 << 1)
> > +#define CCID_PINSUPOORT_MODIFICATION (1 << 2)
>
> --
> With Best Regards,
> Andy Shevchenko
Best regards,
Marcus Folkesson
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v3 1/3] usb: gadget: ccid: add support for USB CCID Gadget Device
From: Greg Kroah-Hartman @ 2018-05-30 11:30 UTC (permalink / raw)
To: Marcus Folkesson
Cc: Andy Shevchenko, Jonathan Corbet, Felipe Balbi, David S. Miller,
Mauro Carvalho Chehab, Andrew Morton, Randy Dunlap,
Ruslan Bilovol, Thomas Gleixner, Kate Stewart, USB,
Linux Documentation List, Linux Kernel Mailing List
In-Reply-To: <20180530112459.GB2939@gmail.com>
On Wed, May 30, 2018 at 01:24:59PM +0200, Marcus Folkesson wrote:
> Hi Andy,
>
> Thank you for your comments!
> Many good catches here!
>
> On Wed, May 30, 2018 at 03:55:39AM +0300, Andy Shevchenko wrote:
> > On Tue, May 29, 2018 at 9:50 PM, Marcus Folkesson
> > <marcus.folkesson@gmail.com> wrote:
> > > Chip Card Interface Device (CCID) protocol is a USB protocol that
> > > allows a smartcard device to be connected to a computer via a card
> > > reader using a standard USB interface, without the need for each manufacturer
> > > of smartcards to provide its own reader or protocol.
> > >
> > > This gadget driver makes Linux show up as a CCID device to the host and let a
> > > userspace daemon act as the smartcard.
> > >
> > > This is useful when the Linux gadget itself should act as a cryptographic
> > > device or forward APDUs to an embedded smartcard device.
> >
> > > + * Copyright (C) 2018 Marcus Folkesson <marcus.folkesson@gmail.com>
> >
> > > + *
> >
> > Redundant line
> >
>
> Yep
>
> > > +static DEFINE_IDA(ccidg_ida);
> >
> > Where is it destroyed?
>
> Hm, I'm not sure it needs to be destroyed. From lib/idr.c:
>
> * You can also use ida_get_new_above() if you need an ID to be allocated
> * above a particular number. ida_destroy() can be used to dispose of an
> * IDA without needing to free the individual IDs in it. You can use
> * ida_is_empty() to find out whether the IDA has any IDs currently allocated.
>
>
> An empty ccidg_ida is the indication that we should clean up our
> mess:
>
> static void ccidg_free_inst(struct usb_function_instance *f)
> ...
> if (ida_is_empty(&ccidg_ida))
> ccidg_cleanup();
>
> If the IDA is empty, should I call ida_destroy() anyway?
> Other similiar drivers does not seems to do that.
>
> I must say that I'm not very familiar with the IDA API.
When your module is removed, you need to clean up any remaining memory
that the ida used. It's not obvious at all, and is a pain as you would
think that if you statically allocate one, like you have here, it would
not be needed. You need to just call:
ida_destroy(&ccidg_ida);
in your module exit function.
Hope this helps,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v3 2/3] Documentation: usb: add documentation for USB CCID Gadget Device
From: Marcus Folkesson @ 2018-05-30 11:34 UTC (permalink / raw)
To: Randy Dunlap
Cc: Greg Kroah-Hartman, Jonathan Corbet, Felipe Balbi, davem,
Mauro Carvalho Chehab, Andrew Morton, Ruslan Bilovol,
Thomas Gleixner, Kate Stewart, linux-usb, linux-doc, linux-kernel
In-Reply-To: <f4cdc62c-8bbe-28f9-0e7a-b4943d6f6a4f@infradead.org>
Hi Randy,
On Tue, May 29, 2018 at 01:27:23PM -0700, Randy Dunlap wrote:
> On 05/29/2018 11:50 AM, Marcus Folkesson wrote:
> > Add documentation to give a brief description on how to use the
> > CCID Gadget Device.
> > This includes a description for all attributes followed by an example on
> > how to setup the device with ConfigFS.
> >
> > Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> > ---
> >
> > Notes:
> > v3:
> > - correct the grammer (thanks Randy)
> > v2:
> > - add the missing changelog text
> >
> > Documentation/usb/gadget_ccid.rst | 267 ++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 267 insertions(+)
> > create mode 100644 Documentation/usb/gadget_ccid.rst
> >
> > diff --git a/Documentation/usb/gadget_ccid.rst b/Documentation/usb/gadget_ccid.rst
> > new file mode 100644
> > index 000000000000..524fe9e6ac19
> > --- /dev/null
> > +++ b/Documentation/usb/gadget_ccid.rst
> > @@ -0,0 +1,267 @@
> > +.. SPDX-License-Identifier: GPL-2.0
> > +
> > +============
> > +CCID Gadget
> > +============
> > +
> > +:Author: Marcus Folkesson <marcus.folkesson@gmail.com>
> > +
> > +Introduction
> > +============
> > +
> > +The CCID Gadget will present itself as a CCID device to the host system.
> > +The device supports two endpoints for now; BULK IN and BULK OUT.
> > +These endpoints are exposed to userspace via /dev/ccidg*.
> > +
> > +All CCID commands are sent on the BULK-OUT endpoint. Each command sent to the CCID
> > +has an associated ending response. Some commands can also have intermediate
> > +responses. The response is sent on the BULK-IN endpoint.
> > +See Figure 3-3 in the CCID Specification [1]_ for more details.
> > +
> > +The CCID commands must be handled in userspace since the driver is only working
> > +as a transport layer for the TPDUs.
>
> I think that it would be helpful to tell us what the naming of the /dev/ccidg*
> endpoints looks like. Also, how to distinguish the BULK-IN from the BULK-OUT
> endpoint.
You are right, it is not clear.
The devices are named as /dev/ccidg[0-4] and is per device.
Writing/reading to the device is using BULK-IN/BULK-OUT, the /dev/ccidg*
device is not bound to a particular endpoint.
I will come up with something that make it more clear.
Thank you!
>
> > +
> > +
> > +CCID Commands
> > +--------------
> > +
> > +All CCID commands begins with a 10-byte header followed by an optional
> > +data field depending on message type.
> > +
> > ++--------+--------------+-------+----------------------------------+
> > +| Offset | Field | Size | Description |
> > ++========+==============+=======+==================================+
> > +| 0 | bMessageType | 1 | Type of message |
> > ++--------+--------------+-------+----------------------------------+
> > +| 1 | dwLength | 4 | Message specific data length |
> > +| | | | |
> > ++--------+--------------+-------+----------------------------------+
> > +| 5 | bSlot | 1 | Identifies the slot number |
> > +| | | | for this command |
> > ++--------+--------------+-------+----------------------------------+
> > +| 6 | bSeq | 1 | Sequence number for command |
> > ++--------+--------------+-------+----------------------------------+
> > +| 7 | ... | 3 | Fields depends on message type |
> > ++--------+--------------+-------+----------------------------------+
> > +| 10 | abData | array | Message specific data (OPTIONAL) |
> > ++--------+--------------+-------+----------------------------------+
> > +
> > +
> > +Multiple CCID gadgets
> > +----------------------
> > +
> > +It is possible to create multiple instances of the CCID gadget, however,
> > +a much more flexible way is to create one gadget and set the `nslots` attribute
> > +to the number of desired CCID devices.
> > +
> > +All CCID commands specify which slot is the receiver in the `bSlot` field
> > +of the CCID header.
> > +
> > +Usage
> > +=====
> > +
> > +Access from userspace
> > +----------------------
> > +All communication is by read(2) and write(2) to the corresponding /dev/ccidg* device.
> > +Only one file descriptor is allowed to be open to the device at a time.
>
>
> Reviewed-by: Randy Dunlap <rdunlap@infradead.org>
>
>
> thanks,
> --
> ~Randy
Best regards,
Marcus Folkesson
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v3 1/3] usb: gadget: ccid: add support for USB CCID Gadget Device
From: Marcus Folkesson @ 2018-05-30 12:13 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Andy Shevchenko, Jonathan Corbet, Felipe Balbi, David S. Miller,
Mauro Carvalho Chehab, Andrew Morton, Randy Dunlap,
Ruslan Bilovol, Thomas Gleixner, Kate Stewart, USB,
Linux Documentation List, Linux Kernel Mailing List
In-Reply-To: <20180530113026.GA20775@kroah.com>
Hi Greg,
On Wed, May 30, 2018 at 01:30:26PM +0200, Greg Kroah-Hartman wrote:
> On Wed, May 30, 2018 at 01:24:59PM +0200, Marcus Folkesson wrote:
> > Hi Andy,
> >
> > Thank you for your comments!
> > Many good catches here!
> >
> > On Wed, May 30, 2018 at 03:55:39AM +0300, Andy Shevchenko wrote:
> > > On Tue, May 29, 2018 at 9:50 PM, Marcus Folkesson
> > > <marcus.folkesson@gmail.com> wrote:
> > > > Chip Card Interface Device (CCID) protocol is a USB protocol that
> > > > allows a smartcard device to be connected to a computer via a card
> > > > reader using a standard USB interface, without the need for each manufacturer
> > > > of smartcards to provide its own reader or protocol.
> > > >
> > > > This gadget driver makes Linux show up as a CCID device to the host and let a
> > > > userspace daemon act as the smartcard.
> > > >
> > > > This is useful when the Linux gadget itself should act as a cryptographic
> > > > device or forward APDUs to an embedded smartcard device.
> > >
> > > > + * Copyright (C) 2018 Marcus Folkesson <marcus.folkesson@gmail.com>
> > >
> > > > + *
> > >
> > > Redundant line
> > >
> >
> > Yep
> >
> > > > +static DEFINE_IDA(ccidg_ida);
> > >
> > > Where is it destroyed?
> >
> > Hm, I'm not sure it needs to be destroyed. From lib/idr.c:
> >
> > * You can also use ida_get_new_above() if you need an ID to be allocated
> > * above a particular number. ida_destroy() can be used to dispose of an
> > * IDA without needing to free the individual IDs in it. You can use
> > * ida_is_empty() to find out whether the IDA has any IDs currently allocated.
> >
> >
> > An empty ccidg_ida is the indication that we should clean up our
> > mess:
> >
> > static void ccidg_free_inst(struct usb_function_instance *f)
> > ...
> > if (ida_is_empty(&ccidg_ida))
> > ccidg_cleanup();
> >
> > If the IDA is empty, should I call ida_destroy() anyway?
> > Other similiar drivers does not seems to do that.
> >
> > I must say that I'm not very familiar with the IDA API.
>
> When your module is removed, you need to clean up any remaining memory
> that the ida used. It's not obvious at all, and is a pain as you would
> think that if you statically allocate one, like you have here, it would
> not be needed. You need to just call:
> ida_destroy(&ccidg_ida);
> in your module exit function.
>
> Hope this helps,
Thank you for making it clear.
Maybe I should use
#define DECLARE_USB_FUNCTION(_name, _inst_alloc, _func_alloc) \
instead of
#define DECLARE_USB_FUNCTION_INIT(_name, _inst_alloc, _func_alloc) \
and provide my own module_init/module_exit functions then?
Just give me a hint and I will do the same for
f_printer.c and
f_hid.c
as those use IDAs in a similiar way.
>
> greg k-h
Best regards,
Marcus Folkesson
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v3 1/3] usb: gadget: ccid: add support for USB CCID Gadget Device
From: Greg Kroah-Hartman @ 2018-05-30 12:20 UTC (permalink / raw)
To: Marcus Folkesson
Cc: Andy Shevchenko, Jonathan Corbet, Felipe Balbi, David S. Miller,
Mauro Carvalho Chehab, Andrew Morton, Randy Dunlap,
Ruslan Bilovol, Thomas Gleixner, Kate Stewart, USB,
Linux Documentation List, Linux Kernel Mailing List
In-Reply-To: <20180530121357.GD2939@gmail.com>
On Wed, May 30, 2018 at 02:13:57PM +0200, Marcus Folkesson wrote:
> Hi Greg,
>
> On Wed, May 30, 2018 at 01:30:26PM +0200, Greg Kroah-Hartman wrote:
> > On Wed, May 30, 2018 at 01:24:59PM +0200, Marcus Folkesson wrote:
> > > Hi Andy,
> > >
> > > Thank you for your comments!
> > > Many good catches here!
> > >
> > > On Wed, May 30, 2018 at 03:55:39AM +0300, Andy Shevchenko wrote:
> > > > On Tue, May 29, 2018 at 9:50 PM, Marcus Folkesson
> > > > <marcus.folkesson@gmail.com> wrote:
> > > > > Chip Card Interface Device (CCID) protocol is a USB protocol that
> > > > > allows a smartcard device to be connected to a computer via a card
> > > > > reader using a standard USB interface, without the need for each manufacturer
> > > > > of smartcards to provide its own reader or protocol.
> > > > >
> > > > > This gadget driver makes Linux show up as a CCID device to the host and let a
> > > > > userspace daemon act as the smartcard.
> > > > >
> > > > > This is useful when the Linux gadget itself should act as a cryptographic
> > > > > device or forward APDUs to an embedded smartcard device.
> > > >
> > > > > + * Copyright (C) 2018 Marcus Folkesson <marcus.folkesson@gmail.com>
> > > >
> > > > > + *
> > > >
> > > > Redundant line
> > > >
> > >
> > > Yep
> > >
> > > > > +static DEFINE_IDA(ccidg_ida);
> > > >
> > > > Where is it destroyed?
> > >
> > > Hm, I'm not sure it needs to be destroyed. From lib/idr.c:
> > >
> > > * You can also use ida_get_new_above() if you need an ID to be allocated
> > > * above a particular number. ida_destroy() can be used to dispose of an
> > > * IDA without needing to free the individual IDs in it. You can use
> > > * ida_is_empty() to find out whether the IDA has any IDs currently allocated.
> > >
> > >
> > > An empty ccidg_ida is the indication that we should clean up our
> > > mess:
> > >
> > > static void ccidg_free_inst(struct usb_function_instance *f)
> > > ...
> > > if (ida_is_empty(&ccidg_ida))
> > > ccidg_cleanup();
> > >
> > > If the IDA is empty, should I call ida_destroy() anyway?
> > > Other similiar drivers does not seems to do that.
> > >
> > > I must say that I'm not very familiar with the IDA API.
> >
> > When your module is removed, you need to clean up any remaining memory
> > that the ida used. It's not obvious at all, and is a pain as you would
> > think that if you statically allocate one, like you have here, it would
> > not be needed. You need to just call:
> > ida_destroy(&ccidg_ida);
> > in your module exit function.
> >
> > Hope this helps,
>
> Thank you for making it clear.
>
> Maybe I should use
> #define DECLARE_USB_FUNCTION(_name, _inst_alloc, _func_alloc) \
>
> instead of
> #define DECLARE_USB_FUNCTION_INIT(_name, _inst_alloc, _func_alloc) \
>
> and provide my own module_init/module_exit functions then?
Probably, yes.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v3 1/3] usb: gadget: ccid: add support for USB CCID Gadget Device
From: Felipe Balbi @ 2018-05-30 12:28 UTC (permalink / raw)
To: Marcus Folkesson, Greg Kroah-Hartman, Jonathan Corbet, davem,
Mauro Carvalho Chehab, Andrew Morton, Randy Dunlap,
Ruslan Bilovol, Thomas Gleixner, Kate Stewart
Cc: linux-usb, linux-doc, linux-kernel, Marcus Folkesson
In-Reply-To: <20180529185021.13738-1-marcus.folkesson@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 803 bytes --]
Marcus Folkesson <marcus.folkesson@gmail.com> writes:
> Chip Card Interface Device (CCID) protocol is a USB protocol that
> allows a smartcard device to be connected to a computer via a card
> reader using a standard USB interface, without the need for each manufacturer
> of smartcards to provide its own reader or protocol.
>
> This gadget driver makes Linux show up as a CCID device to the host and let a
> userspace daemon act as the smartcard.
>
> This is useful when the Linux gadget itself should act as a cryptographic
> device or forward APDUs to an embedded smartcard device.
>
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
this could be done entirely in userspace with functionfs, why do we need
this part in the kernel? It does very little.
--
balbi
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply
* Re: [PATCH v9 0/7] Enable cpuset controller in default hierarchy
From: Waiman Long @ 2018-05-30 12:56 UTC (permalink / raw)
To: Juri Lelli
Cc: Tejun Heo, Li Zefan, Johannes Weiner, Peter Zijlstra, Ingo Molnar,
cgroups, linux-kernel, linux-doc, kernel-team, pjt, luto,
Mike Galbraith, torvalds, Roman Gushchin, Patrick Bellasi
In-Reply-To: <20180530101317.GB3320@localhost.localdomain>
On 05/30/2018 06:13 AM, Juri Lelli wrote:
> Hi,
>
> On 29/05/18 09:41, Waiman Long wrote:
>> v9:
>> - Rename cpuset.sched.domain to cpuset.sched.domain_root to better
>> identify its purpose as the root of a new scheduling domain or
>> partition.
>> - Clarify in the document about the purpose of domain_root and
>> load_balance. Using domain_root is th only way to create new
>> partition.
>> - Fix a lockdep warning in update_isolated_cpumask() function.
>> - Add a new patch to eliminate call to generate_sched_domains() for
>> v2 when a change in cpu list does not touch a domain_root.
> I was playing with this and ended up with the situation below:
>
> g1/cgroup.controllers:cpuset
> g1/cgroup.events:populated 0
> g1/cgroup.max.depth:max
> g1/cgroup.max.descendants:max
> g1/cgroup.stat:nr_descendants 1
> g1/cgroup.stat:nr_dying_descendants 0
> g1/cgroup.subtree_control:cpuset
> g1/cgroup.type:domain
> g1/cpuset.cpus:0-5 <---
> g1/cpuset.cpus.effective:0-5
> g1/cpuset.mems.effective:0-1
> g1/cpuset.sched.domain_root:1 <---
> g1/cpuset.sched.load_balance:1
> g1/cpu.stat:usage_usec 0
> g1/cpu.stat:user_usec 0
> g1/cpu.stat:system_usec 0
> g1/g11/cgroup.events:populated 0
> g1/g11/cgroup.max.descendants:max
> g1/g11/cpu.stat:usage_usec 0
> g1/g11/cpu.stat:user_usec 0
> g1/g11/cpu.stat:system_usec 0
> g1/g11/cgroup.type:domain
> g1/g11/cgroup.stat:nr_descendants 0
> g1/g11/cgroup.stat:nr_dying_descendants 0
> g1/g11/cpuset.cpus.effective:0-5
> g1/g11/cgroup.controllers:cpuset
> g1/g11/cpuset.sched.load_balance:1
> g1/g11/cpuset.mems.effective:0-1
> g1/g11/cpuset.cpus:6-11 <---
> g1/g11/cgroup.max.depth:max
> g1/g11/cpuset.sched.domain_root:0
>
> Should this be allowed? I was expecting subgroup g11 should be
> restricted to a subset of g1's cpus.
>
> Best,
>
> - Juri
That shouldn't be allowed.The code is probably missing some checks that
should have been done. What was the sequence of commands leading to the
above configuration?
Cheers,
Longman
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v9 0/7] Enable cpuset controller in default hierarchy
From: Juri Lelli @ 2018-05-30 13:05 UTC (permalink / raw)
To: Waiman Long
Cc: Tejun Heo, Li Zefan, Johannes Weiner, Peter Zijlstra, Ingo Molnar,
cgroups, linux-kernel, linux-doc, kernel-team, pjt, luto,
Mike Galbraith, torvalds, Roman Gushchin, Patrick Bellasi
In-Reply-To: <d74c67ea-43c9-cabb-4303-ecff008412df@redhat.com>
On 30/05/18 08:56, Waiman Long wrote:
> On 05/30/2018 06:13 AM, Juri Lelli wrote:
> > Hi,
> >
> > On 29/05/18 09:41, Waiman Long wrote:
> >> v9:
> >> - Rename cpuset.sched.domain to cpuset.sched.domain_root to better
> >> identify its purpose as the root of a new scheduling domain or
> >> partition.
> >> - Clarify in the document about the purpose of domain_root and
> >> load_balance. Using domain_root is th only way to create new
> >> partition.
> >> - Fix a lockdep warning in update_isolated_cpumask() function.
> >> - Add a new patch to eliminate call to generate_sched_domains() for
> >> v2 when a change in cpu list does not touch a domain_root.
> > I was playing with this and ended up with the situation below:
> >
> > g1/cgroup.controllers:cpuset
> > g1/cgroup.events:populated 0
> > g1/cgroup.max.depth:max
> > g1/cgroup.max.descendants:max
> > g1/cgroup.stat:nr_descendants 1
> > g1/cgroup.stat:nr_dying_descendants 0
> > g1/cgroup.subtree_control:cpuset
> > g1/cgroup.type:domain
> > g1/cpuset.cpus:0-5 <---
> > g1/cpuset.cpus.effective:0-5
> > g1/cpuset.mems.effective:0-1
> > g1/cpuset.sched.domain_root:1 <---
> > g1/cpuset.sched.load_balance:1
> > g1/cpu.stat:usage_usec 0
> > g1/cpu.stat:user_usec 0
> > g1/cpu.stat:system_usec 0
> > g1/g11/cgroup.events:populated 0
> > g1/g11/cgroup.max.descendants:max
> > g1/g11/cpu.stat:usage_usec 0
> > g1/g11/cpu.stat:user_usec 0
> > g1/g11/cpu.stat:system_usec 0
> > g1/g11/cgroup.type:domain
> > g1/g11/cgroup.stat:nr_descendants 0
> > g1/g11/cgroup.stat:nr_dying_descendants 0
> > g1/g11/cpuset.cpus.effective:0-5
> > g1/g11/cgroup.controllers:cpuset
> > g1/g11/cpuset.sched.load_balance:1
> > g1/g11/cpuset.mems.effective:0-1
> > g1/g11/cpuset.cpus:6-11 <---
> > g1/g11/cgroup.max.depth:max
> > g1/g11/cpuset.sched.domain_root:0
> >
> > Should this be allowed? I was expecting subgroup g11 should be
> > restricted to a subset of g1's cpus.
> >
> > Best,
> >
> > - Juri
>
> That shouldn't be allowed.The code is probably missing some checks that
> should have been done. What was the sequence of commands leading to the
> above configuration?
This is a E5-2609 v3 (12 cores) Fedora Server box (with systemd, so
first command is needed to be able to use cpuset controller with v2,
IIUC):
# umount /sys/fs/cgroup/cpuset
# cd /sys/fs/cgroup/unified/
# echo "+cpuset" >cgroup.subtree_control
# mkdir g1
# echo 0-5 >g1/cpuset.cpus
# echo 6-11 >init.scope/cpuset.cpus
# echo 6-11 >machine.slice/cpuset.cpus
# echo 6-11 >system.slice/cpuset.cpus
# echo 6-11 >user.slice/cpuset.cpus
# echo 1 >g1/cpuset.sched.domain_root
# mkdir g1/g11
# echo "+cpuset" > g1/cgroup.subtree_control
# echo 6-11 >g1/g11/cpuset.cpus
# grep -R . g1/*
That should be it. Am I doing something wrong?
Thanks,
- Juri
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH] cpuset: Enforce that a child's cpus must be a subset of the parent
From: Waiman Long @ 2018-05-30 13:46 UTC (permalink / raw)
To: Tejun Heo, Li Zefan, Johannes Weiner, Peter Zijlstra, Ingo Molnar
Cc: cgroups, linux-kernel, linux-doc, kernel-team, pjt, luto,
Mike Galbraith, torvalds, Roman Gushchin, Juri Lelli,
Patrick Bellasi, Waiman Long
It was found that the cpuset.cpus could contain CPUs that are not listed
in their parent's cpu list as shown by the command sequence below:
# echo "+cpuset" >cgroup.subtree_control
# mkdir g1
# echo 0-5 >g1/cpuset.cpus
# mkdir g1/g11
# echo "+cpuset" > g1/cgroup.subtree_control
# echo 6-11 >g1/g11/cpuset.cpus
# grep -R . g1 | grep "\.cpus"
g1/cpuset.cpus:0-5
g1/cpuset.cpus.effective:0-5
g1/g11/cpuset.cpus:6-11
g1/g11/cpuset.cpus.effective:0-5
As the intersection of g11's cpus and that of g1 is empty, the effective
cpus of g11 is just that of g1. The check in update_cpumask() is now
corrected to make sure that cpus in a child cpus must be a subset of
its parent's cpus. The error "write error: Invalid argument" will now
be reported in the above case.
Reported-by: Juri Lelli <juri.lelli@redhat.com>
Signed-off-by: Waiman Long <longman@redhat.com>
---
kernel/cgroup/cpuset.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
index 71fb2d0..ceec438 100644
--- a/kernel/cgroup/cpuset.c
+++ b/kernel/cgroup/cpuset.c
@@ -1185,12 +1185,17 @@ static int update_cpumask(struct cpuset *cs, struct cpuset *trialcs,
if (!*buf) {
cpumask_clear(trialcs->cpus_allowed);
} else {
+ struct cpuset *parent = parent_cs(cs);
+
retval = cpulist_parse(buf, trialcs->cpus_allowed);
if (retval < 0)
return retval;
+ /*
+ * The cpu list must be a subset of the parent.
+ */
if (!cpumask_subset(trialcs->cpus_allowed,
- top_cpuset.cpus_allowed))
+ parent->cpus_allowed))
return -EINVAL;
}
--
1.8.3.1
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* Re: [PATCH v9 0/7] Enable cpuset controller in default hierarchy
From: Waiman Long @ 2018-05-30 13:47 UTC (permalink / raw)
To: Juri Lelli
Cc: Tejun Heo, Li Zefan, Johannes Weiner, Peter Zijlstra, Ingo Molnar,
cgroups, linux-kernel, linux-doc, kernel-team, pjt, luto,
Mike Galbraith, torvalds, Roman Gushchin, Patrick Bellasi
In-Reply-To: <20180530130555.GD3320@localhost.localdomain>
On 05/30/2018 09:05 AM, Juri Lelli wrote:
> On 30/05/18 08:56, Waiman Long wrote:
>> On 05/30/2018 06:13 AM, Juri Lelli wrote:
>>> Hi,
>>>
>>> On 29/05/18 09:41, Waiman Long wrote:
>>>> v9:
>>>> - Rename cpuset.sched.domain to cpuset.sched.domain_root to better
>>>> identify its purpose as the root of a new scheduling domain or
>>>> partition.
>>>> - Clarify in the document about the purpose of domain_root and
>>>> load_balance. Using domain_root is th only way to create new
>>>> partition.
>>>> - Fix a lockdep warning in update_isolated_cpumask() function.
>>>> - Add a new patch to eliminate call to generate_sched_domains() for
>>>> v2 when a change in cpu list does not touch a domain_root.
>>> I was playing with this and ended up with the situation below:
>>>
>>> g1/cgroup.controllers:cpuset
>>> g1/cgroup.events:populated 0
>>> g1/cgroup.max.depth:max
>>> g1/cgroup.max.descendants:max
>>> g1/cgroup.stat:nr_descendants 1
>>> g1/cgroup.stat:nr_dying_descendants 0
>>> g1/cgroup.subtree_control:cpuset
>>> g1/cgroup.type:domain
>>> g1/cpuset.cpus:0-5 <---
>>> g1/cpuset.cpus.effective:0-5
>>> g1/cpuset.mems.effective:0-1
>>> g1/cpuset.sched.domain_root:1 <---
>>> g1/cpuset.sched.load_balance:1
>>> g1/cpu.stat:usage_usec 0
>>> g1/cpu.stat:user_usec 0
>>> g1/cpu.stat:system_usec 0
>>> g1/g11/cgroup.events:populated 0
>>> g1/g11/cgroup.max.descendants:max
>>> g1/g11/cpu.stat:usage_usec 0
>>> g1/g11/cpu.stat:user_usec 0
>>> g1/g11/cpu.stat:system_usec 0
>>> g1/g11/cgroup.type:domain
>>> g1/g11/cgroup.stat:nr_descendants 0
>>> g1/g11/cgroup.stat:nr_dying_descendants 0
>>> g1/g11/cpuset.cpus.effective:0-5
>>> g1/g11/cgroup.controllers:cpuset
>>> g1/g11/cpuset.sched.load_balance:1
>>> g1/g11/cpuset.mems.effective:0-1
>>> g1/g11/cpuset.cpus:6-11 <---
>>> g1/g11/cgroup.max.depth:max
>>> g1/g11/cpuset.sched.domain_root:0
>>>
>>> Should this be allowed? I was expecting subgroup g11 should be
>>> restricted to a subset of g1's cpus.
>>>
>>> Best,
>>>
>>> - Juri
>> That shouldn't be allowed.The code is probably missing some checks that
>> should have been done. What was the sequence of commands leading to the
>> above configuration?
> This is a E5-2609 v3 (12 cores) Fedora Server box (with systemd, so
> first command is needed to be able to use cpuset controller with v2,
> IIUC):
>
> # umount /sys/fs/cgroup/cpuset
> # cd /sys/fs/cgroup/unified/
> # echo "+cpuset" >cgroup.subtree_control
> # mkdir g1
> # echo 0-5 >g1/cpuset.cpus
> # echo 6-11 >init.scope/cpuset.cpus
> # echo 6-11 >machine.slice/cpuset.cpus
> # echo 6-11 >system.slice/cpuset.cpus
> # echo 6-11 >user.slice/cpuset.cpus
> # echo 1 >g1/cpuset.sched.domain_root
> # mkdir g1/g11
> # echo "+cpuset" > g1/cgroup.subtree_control
> # echo 6-11 >g1/g11/cpuset.cpus
> # grep -R . g1/*
>
> That should be it. Am I doing something wrong?
>
> Thanks,
>
> - Juri
Yes, it is a bug in the existing code. I have sent out a patch to fix that.
-Longman
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v9 0/7] Enable cpuset controller in default hierarchy
From: Juri Lelli @ 2018-05-30 13:52 UTC (permalink / raw)
To: Waiman Long
Cc: Tejun Heo, Li Zefan, Johannes Weiner, Peter Zijlstra, Ingo Molnar,
cgroups, linux-kernel, linux-doc, kernel-team, pjt, luto,
Mike Galbraith, torvalds, Roman Gushchin, Patrick Bellasi
In-Reply-To: <e42f910a-83b7-0fd2-2c77-05d069441c2f@redhat.com>
On 30/05/18 09:47, Waiman Long wrote:
[...]
> Yes, it is a bug in the existing code. I have sent out a patch to fix that.
Cool, thanks. I'll have a look.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] cpuset: Enforce that a child's cpus must be a subset of the parent
From: Juri Lelli @ 2018-05-30 14:00 UTC (permalink / raw)
To: Waiman Long
Cc: Tejun Heo, Li Zefan, Johannes Weiner, Peter Zijlstra, Ingo Molnar,
cgroups, linux-kernel, linux-doc, kernel-team, pjt, luto,
Mike Galbraith, torvalds, Roman Gushchin, Patrick Bellasi
In-Reply-To: <1527687991-1431-1-git-send-email-longman@redhat.com>
Hi,
On 30/05/18 09:46, Waiman Long wrote:
> It was found that the cpuset.cpus could contain CPUs that are not listed
> in their parent's cpu list as shown by the command sequence below:
>
> # echo "+cpuset" >cgroup.subtree_control
> # mkdir g1
> # echo 0-5 >g1/cpuset.cpus
> # mkdir g1/g11
> # echo "+cpuset" > g1/cgroup.subtree_control
> # echo 6-11 >g1/g11/cpuset.cpus
> # grep -R . g1 | grep "\.cpus"
> g1/cpuset.cpus:0-5
> g1/cpuset.cpus.effective:0-5
> g1/g11/cpuset.cpus:6-11
> g1/g11/cpuset.cpus.effective:0-5
>
> As the intersection of g11's cpus and that of g1 is empty, the effective
> cpus of g11 is just that of g1. The check in update_cpumask() is now
> corrected to make sure that cpus in a child cpus must be a subset of
> its parent's cpus. The error "write error: Invalid argument" will now
> be reported in the above case.
>
> Reported-by: Juri Lelli <juri.lelli@redhat.com>
> Signed-off-by: Waiman Long <longman@redhat.com>
Looks like it fixes the bug.
Reviewed-and-Tested-by: Juri Lelli <juri.lelli@redhat.com>
Thanks,
- Juri
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v3 1/3] usb: gadget: ccid: add support for USB CCID Gadget Device
From: Marcus Folkesson @ 2018-05-30 14:04 UTC (permalink / raw)
To: Felipe Balbi
Cc: Greg Kroah-Hartman, Jonathan Corbet, davem, Mauro Carvalho Chehab,
Andrew Morton, Randy Dunlap, Ruslan Bilovol, Thomas Gleixner,
Kate Stewart, linux-usb, linux-doc, linux-kernel
In-Reply-To: <87r2ltcopp.fsf@linux.intel.com>
Hi Filipe,
On Wed, May 30, 2018 at 03:28:18PM +0300, Felipe Balbi wrote:
> Marcus Folkesson <marcus.folkesson@gmail.com> writes:
>
> > Chip Card Interface Device (CCID) protocol is a USB protocol that
> > allows a smartcard device to be connected to a computer via a card
> > reader using a standard USB interface, without the need for each manufacturer
> > of smartcards to provide its own reader or protocol.
> >
> > This gadget driver makes Linux show up as a CCID device to the host and let a
> > userspace daemon act as the smartcard.
> >
> > This is useful when the Linux gadget itself should act as a cryptographic
> > device or forward APDUs to an embedded smartcard device.
> >
> > Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
>
> this could be done entirely in userspace with functionfs, why do we need
> this part in the kernel? It does very little.
Andrzej pointed this out, and I actually do not have any good answer
more than that the userspace application could be kept small and the
important configuration of the CCID device is done with well (I hope)
documented configfs attributes.
>
> --
> balbi
Best regards,
Marcus Folkesson
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v9 2/7] cpuset: Add new v2 cpuset.sched.domain_root flag
From: Juri Lelli @ 2018-05-30 14:18 UTC (permalink / raw)
To: Waiman Long
Cc: Tejun Heo, Li Zefan, Johannes Weiner, Peter Zijlstra, Ingo Molnar,
cgroups, linux-kernel, linux-doc, kernel-team, pjt, luto,
Mike Galbraith, torvalds, Roman Gushchin, Patrick Bellasi
In-Reply-To: <1527601294-3444-3-git-send-email-longman@redhat.com>
Hi,
On 29/05/18 09:41, Waiman Long wrote:
[...]
> + cpuset.sched.domain_root
> + A read-write single value file which exists on non-root
> + cpuset-enabled cgroups. It is a binary value flag that accepts
> + either "0" (off) or "1" (on). This flag is set by the parent
> + and is not delegatable.
> +
> + If set, it indicates that the current cgroup is the root of a
> + new scheduling domain or partition that comprises itself and
> + all its descendants except those that are scheduling domain
> + roots themselves and their descendants. The root cgroup is
> + always a scheduling domain root.
> +
> + There are constraints on where this flag can be set. It can
> + only be set in a cgroup if all the following conditions are true.
> +
> + 1) The "cpuset.cpus" is not empty and the list of CPUs are
> + exclusive, i.e. they are not shared by any of its siblings.
> + 2) The parent cgroup is also a scheduling domain root.
> + 3) There is no child cgroups with cpuset enabled. This is
> + for eliminating corner cases that have to be handled if such
> + a condition is allowed.
> +
> + Setting this flag will take the CPUs away from the effective
> + CPUs of the parent cgroup. Once it is set, this flag cannot
> + be cleared if there are any child cgroups with cpuset enabled.
> + Further changes made to "cpuset.cpus" is allowed as long as
> + the first condition above is still true.
IIUC, with the configuration below
cpuset.cpus.effective:6-11
cgroup.controllers:cpuset
cpuset.mems.effective:0-1
cgroup.subtree_control:cpuset
g1/cpuset.cpus.effective:0-5
g1/cgroup.controllers:cpuset
g1/cpuset.sched.load_balance:1
g1/cpuset.mems.effective:0-1
g1/cpuset.cpus:0-5
g1/cpuset.sched.domain_root:1
user.slice/cpuset.cpus.effective:6-11
user.slice/cgroup.controllers:cpuset
user.slice/cpuset.sched.load_balance:1
user.slice/cpuset.mems.effective:0-1
user.slice/cpuset.cpus:6-11
user.slice/cpuset.sched.domain_root:0
init.scope/cpuset.cpus.effective:6-11
init.scope/cgroup.controllers:cpuset
init.scope/cpuset.sched.load_balance:1
init.scope/cpuset.mems.effective:0-1
init.scope/cpuset.cpus:6-11
init.scope/cpuset.sched.domain_root:0
system.slice/cpuset.cpus.effective:6-11
system.slice/cgroup.controllers:cpuset
system.slice/cpuset.sched.load_balance:1
system.slice/cpuset.mems.effective:0-1
system.slice/cpuset.cpus:6-11
system.slice/cpuset.sched.domain_root:0
machine.slice/cpuset.cpus.effective:6-11
machine.slice/cgroup.controllers:cpuset
machine.slice/cpuset.sched.load_balance:1
machine.slice/cpuset.mems.effective:0-1
machine.slice/cpuset.cpus:6-11
machine.slice/cpuset.sched.domain_root:0
I should be able to
# echo 0-4 >g1/cpuset.cpus
?
It doesn't let me.
I'm not sure we actually want to allow that, but that's what would I
expect as per your text above.
Thanks,
- Juri
BTW: thanks a lot for your prompt feedback and hope it's OK if I keep
playing and asking questions. :)
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox