Re: [PATCH] dma-mapping: Use 'bitmap_zalloc()' when applicable

[Joe Perches]

On Sun, 2021-10-24 at 19:40 +0200, Christophe JAILLET wrote:
> 'dma_mem->bitmap' is a bitmap. So use 'bitmap_zalloc()' to simplify code,
> improve the semantic and avoid some open-coded arithmetic in allocator
> arguments.

There is a cocci script for some of these.

https://lore.kernel.org/all/08b89608cfb1280624d1a89ead6547069f9a4c31.ca...@perches.com/


___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v9 03/17] vdpa: Fix code indentation

[Joe Perches]

On Tue, 2021-07-13 at 16:46 +0800, Xie Yongji wrote:
> Use tabs to indent the code instead of spaces.

There are a lot more of these in this file.

$ ./scripts/checkpatch.pl --fix-inplace --strict include/linux/vdpa.h

and a little typing gives:
---
 include/linux/vdpa.h | 50 +-
 1 file changed, 25 insertions(+), 25 deletions(-)

diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
index 3357ac98878d4..14cd4248e59fd 100644
--- a/include/linux/vdpa.h
+++ b/include/linux/vdpa.h
@@ -43,17 +43,17 @@ struct vdpa_vq_state_split {
  * @last_used_idx: used index
  */
 struct vdpa_vq_state_packed {
-u16last_avail_counter:1;
-u16last_avail_idx:15;
-u16last_used_counter:1;
-u16last_used_idx:15;
+   u16 last_avail_counter:1;
+   u16 last_avail_idx:15;
+   u16 last_used_counter:1;
+   u16 last_used_idx:15;
 };
 
 struct vdpa_vq_state {
- union {
-  struct vdpa_vq_state_split split;
-  struct vdpa_vq_state_packed packed;
- };
+   union {
+   struct vdpa_vq_state_split split;
+   struct vdpa_vq_state_packed packed;
+   };
 };
 
 struct vdpa_mgmt_dev;
@@ -131,7 +131,7 @@ struct vdpa_iova_range {
  * @vdev: vdpa device
  * @idx: virtqueue index
  * @state: pointer to returned state 
(last_avail_idx)
- * @get_vq_notification:   Get the notification area for a virtqueue
+ * @get_vq_notification:   Get the notification area for a virtqueue
  * @vdev: vdpa device
  * @idx: virtqueue index
  * Returns the notifcation area
@@ -277,13 +277,13 @@ struct vdpa_device *__vdpa_alloc_device(struct device 
*parent,
const struct vdpa_config_ops *config,
size_t size, const char *name);
 
-#define vdpa_alloc_device(dev_struct, member, parent, config, name)   \
- container_of(__vdpa_alloc_device( \
-  parent, config, \
-  sizeof(dev_struct) + \
-  BUILD_BUG_ON_ZERO(offsetof( \
-  dev_struct, member)), name), \
-  dev_struct, member)
+#define vdpa_alloc_device(dev_struct, member, parent, config, name)\
+   container_of(__vdpa_alloc_device(parent, config,\
+sizeof(dev_struct) +   \
+BUILD_BUG_ON_ZERO(offsetof(dev_struct, 
\
+   member)), \
+name), \
+dev_struct, member)
 
 int vdpa_register_device(struct vdpa_device *vdev, int nvqs);
 void vdpa_unregister_device(struct vdpa_device *vdev);
@@ -308,8 +308,8 @@ struct vdpa_driver {
 int __vdpa_register_driver(struct vdpa_driver *drv, struct module *owner);
 void vdpa_unregister_driver(struct vdpa_driver *drv);
 
-#define module_vdpa_driver(__vdpa_driver) \
-   module_driver(__vdpa_driver, vdpa_register_driver,  \
+#define module_vdpa_driver(__vdpa_driver)  \
+   module_driver(__vdpa_driver, vdpa_register_driver,  \
  vdpa_unregister_driver)
 
 static inline struct vdpa_driver *drv_to_vdpa(struct device_driver *driver)
@@ -339,25 +339,25 @@ static inline struct device *vdpa_get_dma_dev(struct 
vdpa_device *vdev)
 
 static inline void vdpa_reset(struct vdpa_device *vdev)
 {
-const struct vdpa_config_ops *ops = vdev->config;
+   const struct vdpa_config_ops *ops = vdev->config;
 
vdev->features_valid = false;
-ops->set_status(vdev, 0);
+   ops->set_status(vdev, 0);
 }
 
 static inline int vdpa_set_features(struct vdpa_device *vdev, u64 features)
 {
-const struct vdpa_config_ops *ops = vdev->config;
+   const struct vdpa_config_ops *ops = vdev->config;
 
vdev->features_valid = true;
-return ops->set_features(vdev, features);
+   return ops->set_features(vdev, features);
 }
 
-
-static inline void vdpa_get_config(struct vdpa_device *vdev, unsigned offset,
+static inline void vdpa_get_config(struct vdpa_device *vdev,
+  unsigned int offset,
   void *buf, unsigned int len)
 {
-const struct vdpa_config_ops *ops = vdev->config;
+   const struct vdpa_config_ops *ops = vdev->config;
 
/*
 * Config accesses aren't supposed to trigger before features are set.


___
iommu mailing list
iommu@lists.linux-foundation.org

Re: [PATCH v2] iommu/amd: Fix extended features logging

[Joe Perches]

On Mon, 2021-04-19 at 22:23 +0300, Alexander Monakov wrote:
> On Sun, 11 Apr 2021, Joe Perches wrote:
> 
> > > v2: avoid pr_info(""), change pci_info() to pr_info() for a nicer
> > > solution
> > > 
> > >  drivers/iommu/amd/init.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
> > > index 596d0c413473..62913f82a21f 100644
> > > --- a/drivers/iommu/amd/init.c
> > > +++ b/drivers/iommu/amd/init.c
> > > @@ -1929,8 +1929,8 @@ static void print_iommu_info(void)
> > >   pci_info(pdev, "Found IOMMU cap 0x%hx\n", iommu->cap_ptr);
> > >  
> > > 
> > >   if (iommu->cap & (1 << IOMMU_CAP_EFR)) {
> > > - pci_info(pdev, "Extended features (%#llx):",
> > > -  iommu->features);
> > > + pr_info("Extended features (%#llx):", iommu->features);
> > > +
> > >   for (i = 0; i < ARRAY_SIZE(feat_str); ++i) {
> > >   if (iommu_feature(iommu, (1ULL << i)))
> > >   pr_cont(" %s", feat_str[i]);
> > 
> > How about avoiding all of this by using a temporary buffer
> > and a single pci_info.
> 
> I think it is mostly up to the maintainers, but from my perspective, it's not
> good to conflate such a simple bugfix with the substantial rewrite you are
> proposing (which also increases code complexity).

You and I have _significant_ differences in the definition of substantial.

Buffering the output is the preferred code style in preference to
pr_cont.

Do remember pr_cont should _only_ be used when absolutely necessary
as interleaving of other messages from other processes/threads can
and does occur.



___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2] iommu/amd: Fix extended features logging

[Joe Perches]

On Mon, 2021-04-12 at 00:13 +0300, Alexander Monakov wrote:
> print_iommu_info prints the EFR register and then the decoded list of
> features on a separate line:
> 
> pci :00:00.2: AMD-Vi: Extended features (0x206d73ef22254ade):
>  PPR X2APIC NX GT IA GA PC GA_vAPIC
> 
> The second line is emitted via 'pr_cont', which causes it to have a
> different ('warn') loglevel compared to the previous line ('info').
> 
> Commit 9a295ff0ffc9 attempted to rectify this by removing the newline
> from the pci_info format string, but this doesn't work, as pci_info
> calls implicitly append a newline anyway.
> 
> Printing the decoded features on the same line would make it quite long.
> Instead, change pci_info() to pr_info() to omit PCI bus location info,
> which is shown in the preceding message anyway. This results in:
> 
> pci :00:00.2: AMD-Vi: Found IOMMU cap 0x40
> AMD-Vi: Extended features (0x206d73ef22254ade): PPR X2APIC NX GT IA GA PC 
> GA_vAPIC
> AMD-Vi: Interrupt remapping enabled
> 
> Fixes: 9a295ff0ffc9 ("iommu/amd: Print extended features in one line to fix 
> divergent log levels")
> Link: 
> https://lore.kernel.org/lkml/alpine.lnx.2.20.13.2104112326460.11...@monopod.intra.ispras.ru
> Signed-off-by: Alexander Monakov 
> Cc: Paul Menzel 
> Cc: Joerg Roedel 
> Cc: Suravee Suthikulpanit 
> Cc: iommu@lists.linux-foundation.org
> ---
> 
> v2: avoid pr_info(""), change pci_info() to pr_info() for a nicer
> solution
> 
>  drivers/iommu/amd/init.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
> index 596d0c413473..62913f82a21f 100644
> --- a/drivers/iommu/amd/init.c
> +++ b/drivers/iommu/amd/init.c
> @@ -1929,8 +1929,8 @@ static void print_iommu_info(void)
>   pci_info(pdev, "Found IOMMU cap 0x%hx\n", iommu->cap_ptr);
>  
> 
>   if (iommu->cap & (1 << IOMMU_CAP_EFR)) {
> - pci_info(pdev, "Extended features (%#llx):",
> -  iommu->features);
> + pr_info("Extended features (%#llx):", iommu->features);
> +
>   for (i = 0; i < ARRAY_SIZE(feat_str); ++i) {
>   if (iommu_feature(iommu, (1ULL << i)))
>   pr_cont(" %s", feat_str[i]);

How about avoiding all of this by using a temporary buffer
and a single pci_info.

Miscellanea:
o Move the feat_str and i declarations into the if block for locality

---
 drivers/iommu/amd/init.c | 29 ++---
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 321f5906e6ed..0d219044726e 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -1943,30 +1943,37 @@ static int __init iommu_init_pci(struct amd_iommu 
*iommu)
 
 static void print_iommu_info(void)
 {
-   static const char * const feat_str[] = {
-   "PreF", "PPR", "X2APIC", "NX", "GT", "[5]",
-   "IA", "GA", "HE", "PC"
-   };
struct amd_iommu *iommu;
 
for_each_iommu(iommu) {
struct pci_dev *pdev = iommu->dev;
-   int i;
 
pci_info(pdev, "Found IOMMU cap 0x%x\n", iommu->cap_ptr);
 
if (iommu->cap & (1 << IOMMU_CAP_EFR)) {
-   pci_info(pdev, "Extended features (%#llx):",
-iommu->features);
+   static const char * const feat_str[] = {
+   "PreF", "PPR", "X2APIC", "NX", "GT", "[5]",
+   "IA", "GA", "HE", "PC"
+   };
+   int i;
+   char features[128] = "";
+   int len = 0;
+
for (i = 0; i < ARRAY_SIZE(feat_str); ++i) {
-   if (iommu_feature(iommu, (1ULL << i)))
-   pr_cont(" %s", feat_str[i]);
+   if (!iommu_feature(iommu, BIT_ULL(i)))
+   continue;
+   len += scnprintf(features + len,
+sizeof(features) - len,
+" %s", feat_str[i]);
}
 
if (iommu->features & FEATURE_GAM_VAPIC)
-   pr_cont(" GA_vAPIC");
+   len += scnprintf(features + len,
+sizeof(features) - len,
+" %s", "GA_vPIC");
 
-   pr_cont("\n");
+   pci_info(pdev, "Extended features (%#llx):%s\n",
+iommu->features, features);
}
}
if (irq_remapping_enabled) {


___
iommu mailing list

Re: [PATCH] iommu/amd: Fix extended features logging

[Joe Perches]

On Sun, 2021-04-11 at 21:52 +0200, John Ogness wrote:
> I'd rather fix dev_info callers to allow pr_cont and then fix any code
> that is using this workaround.

Assuming you mean all dev_() uses, me too.

> And if the print maintainers agree it is OK to encourage
> pr_cont(LOGLEVEL "...") usage, then people should really start using
> that if the loglevel on those pieces is important.

I have no stong feeling about the use of pr_cont(
as valuable or not.  I think it's just a trivial bit that
could be somewhat useful when interleaving occurs.

A somewhat better mechanism would be to have an explicit
cookie use like:

cookie = printk_multipart_init(KERN_LEVEL, fmt, ...);
while ()
printk_multipart_cont(cookie, fmt, ...);
printk_multipark_end(cookie, fmt, ...);

And separately, there should be a pr_debug_cont or equivalent.


___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] iommu/amd: Fix extended features logging

[Joe Perches]

(cc'ing the printk maintainers)

On Sun, 2021-04-11 at 00:11 +0300, Alexander Monakov wrote:
> print_iommu_info prints the EFR register and then the decoded list of
> features on a separate line:
> 
> pci :00:00.2: AMD-Vi: Extended features (0x206d73ef22254ade):
>  PPR X2APIC NX GT IA GA PC GA_vAPIC
> 
> The second line is emitted via 'pr_cont', which causes it to have a
> different ('warn') loglevel compared to the previous line ('info').
> 
> Commit 9a295ff0ffc9 attempted to rectify this by removing the newline
> from the pci_info format string, but this doesn't work, as pci_info
> calls implicitly append a newline anyway.
> 
> Restore the newline, and call pr_info with empty format string to set
> the loglevel for subsequent pr_cont calls. The same solution is used in
> EFI and uvesafb drivers.
> 
> Fixes: 9a295ff0ffc9 ("iommu/amd: Print extended features in one line to fix 
> divergent log levels")
> Signed-off-by: Alexander Monakov 
> Cc: Paul Menzel 
> Cc: Joerg Roedel 
> Cc: Suravee Suthikulpanit 
> Cc: iommu@lists.linux-foundation.org
> ---
>  drivers/iommu/amd/init.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
> index 596d0c413473..a25e241eff1c 100644
> --- a/drivers/iommu/amd/init.c
> +++ b/drivers/iommu/amd/init.c
> @@ -1929,8 +1929,11 @@ static void print_iommu_info(void)
>   pci_info(pdev, "Found IOMMU cap 0x%hx\n", iommu->cap_ptr);
>  
> 
>   if (iommu->cap & (1 << IOMMU_CAP_EFR)) {
> - pci_info(pdev, "Extended features (%#llx):",
> + pci_info(pdev, "Extended features (%#llx):\n",
>    iommu->features);
> +
> + pr_info("");
> +
>   for (i = 0; i < ARRAY_SIZE(feat_str); ++i) {
>   if (iommu_feature(iommu, (1ULL << i)))
>   pr_cont(" %s", feat_str[i]);

This shouldn't be necessary.
If this is true then a lot of output logging code broke.



___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [trivial PATCH] treewide: Convert switch/case fallthrough; to break;

[Joe Perches]

On Thu, 2020-09-10 at 15:21 +0100, Robin Murphy wrote:
> On 2020-09-09 21:06, Joe Perches wrote:
> > fallthrough to a separate case/default label break; isn't very readable.
> > 
> > Convert pseudo-keyword fallthrough; statements to a simple break; when
> > the next label is case or default and the only statement in the next
> > label block is break;
> > 
> > Found using:
> > 
> > $ grep-2.5.4 -rP --include=*.[ch] -n 
> > "fallthrough;(\s*(case\s+\w+|default)\s*:\s*){1,7}break;" *
> > 
> > Miscellanea:
> > 
> > o Move or coalesce a couple label blocks above a default: block.
> > 
> > Signed-off-by: Joe Perches 
> > ---
> > 
> > Compiled allyesconfig x86-64 only.
> > A few files for other arches were not compiled.
> > 
> 
> [...]
> > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c 
> > b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > index c192544e874b..743db1abec40 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > @@ -3777,7 +3777,7 @@ static int arm_smmu_device_hw_probe(struct 
> > arm_smmu_device *smmu)
> > switch (FIELD_GET(IDR0_TTF, reg)) {
> > case IDR0_TTF_AARCH32_64:
> > smmu->ias = 40;
> > -   fallthrough;
> > +   break;
> > case IDR0_TTF_AARCH64:
> > break;
> > default:
> 
> I have to say I don't really agree with the readability argument for 
> this one - a fallthrough is semantically correct here, since the first 
> case is a superset of the second. It just happens that anything we would 
> do for the common subset is implicitly assumed (there are other 
> potential cases we simply haven't added support for at the moment), thus 
> the second case is currently empty.
> This change actively obfuscates that distinction.

Then perhaps comments should be added to usefully
describe the mechanisms.

case IDR0_TTF_AARCH32_64:
smmu->ias = 40;
fallthrough;/* and still do the 64 bit processing */
case IDR0_TTF_AARCH64:
/* Nothing specific yet */
break;

> Robin.

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [trivial PATCH] treewide: Convert switch/case fallthrough; to break;

[Joe Perches]

On Wed, 2020-09-09 at 19:36 -0300, Jason Gunthorpe wrote:
> On Wed, Sep 09, 2020 at 01:06:39PM -0700, Joe Perches wrote:
> > fallthrough to a separate case/default label break; isn't very readable.
> > 
> > Convert pseudo-keyword fallthrough; statements to a simple break; when
> > the next label is case or default and the only statement in the next
> > label block is break;
> > 
> > Found using:
> > 
> > $ grep-2.5.4 -rP --include=*.[ch] -n 
> > "fallthrough;(\s*(case\s+\w+|default)\s*:\s*){1,7}break;" *
> > 
> > Miscellanea:
> > 
> > o Move or coalesce a couple label blocks above a default: block.
> > 
> > Signed-off-by: Joe Perches 
> > ---
> > 
> > Compiled allyesconfig x86-64 only.
> > A few files for other arches were not compiled.
> 
> IB part looks OK, I prefer it like this
> 
> You could do the same for continue as well, I saw a few of those..

I saw some continue uses as well but wasn't sure
and didn't look to see if the switch/case with
continue was in a for/while loop.


___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[trivial PATCH] treewide: Convert switch/case fallthrough; to break;

[Joe Perches]

fallthrough to a separate case/default label break; isn't very readable.

Convert pseudo-keyword fallthrough; statements to a simple break; when
the next label is case or default and the only statement in the next
label block is break;

Found using:

$ grep-2.5.4 -rP --include=*.[ch] -n 
"fallthrough;(\s*(case\s+\w+|default)\s*:\s*){1,7}break;" *

Miscellanea:

o Move or coalesce a couple label blocks above a default: block.

Signed-off-by: Joe Perches 
---

Compiled allyesconfig x86-64 only.
A few files for other arches were not compiled.

 arch/arm/mach-mmp/pm-pxa910.c |  2 +-
 arch/arm64/kvm/handle_exit.c  |  2 +-
 arch/mips/kernel/cpu-probe.c  |  2 +-
 arch/mips/math-emu/cp1emu.c   |  2 +-
 arch/s390/pci/pci.c   |  2 +-
 crypto/tcrypt.c   |  4 ++--
 drivers/ata/sata_mv.c |  2 +-
 drivers/atm/lanai.c   |  2 +-
 drivers/gpu/drm/i915/display/intel_sprite.c   |  2 +-
 drivers/gpu/drm/nouveau/nvkm/engine/disp/hdmi.c   |  2 +-
 drivers/hid/wacom_wac.c   |  2 +-
 drivers/i2c/busses/i2c-i801.c |  2 +-
 drivers/infiniband/ulp/rtrs/rtrs-clt.c| 14 +++---
 drivers/infiniband/ulp/rtrs/rtrs-srv.c|  6 +++---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   |  2 +-
 drivers/irqchip/irq-vic.c |  4 ++--
 drivers/md/dm.c   |  2 +-
 drivers/media/dvb-frontends/drxd_hard.c   |  2 +-
 drivers/media/i2c/ov5640.c|  2 +-
 drivers/media/i2c/ov6650.c|  5 ++---
 drivers/media/i2c/smiapp/smiapp-core.c|  2 +-
 drivers/media/i2c/tvp5150.c   |  2 +-
 drivers/media/pci/ddbridge/ddbridge-core.c|  2 +-
 drivers/media/usb/cpia2/cpia2_core.c  |  2 +-
 drivers/mfd/iqs62x.c  |  3 +--
 drivers/mmc/host/atmel-mci.c  |  2 +-
 drivers/mtd/nand/raw/nandsim.c|  2 +-
 drivers/net/ethernet/intel/e1000e/phy.c   |  2 +-
 drivers/net/ethernet/intel/fm10k/fm10k_pf.c   |  2 +-
 drivers/net/ethernet/intel/i40e/i40e_adminq.c |  2 +-
 drivers/net/ethernet/intel/i40e/i40e_txrx.c   |  2 +-
 drivers/net/ethernet/intel/iavf/iavf_txrx.c   |  2 +-
 drivers/net/ethernet/intel/igb/e1000_phy.c|  2 +-
 drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c|  2 +-
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |  2 +-
 drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c|  2 +-
 drivers/net/ethernet/intel/ixgbevf/vf.c   |  2 +-
 drivers/net/ethernet/netronome/nfp/nfpcore/nfp6000_pcie.c |  2 +-
 drivers/net/ethernet/qlogic/qed/qed_mcp.c |  2 +-
 drivers/net/ethernet/sfc/falcon/farch.c   |  2 +-
 drivers/net/ethernet/sfc/farch.c  |  2 +-
 drivers/net/phy/adin.c|  3 +--
 drivers/net/usb/pegasus.c |  4 ++--
 drivers/net/usb/usbnet.c  |  2 +-
 drivers/net/wireless/ath/ath5k/eeprom.c   |  2 +-
 drivers/net/wireless/mediatek/mt7601u/dma.c   |  8 
 drivers/nvme/host/core.c  | 12 ++--
 drivers/pcmcia/db1xxx_ss.c|  4 ++--
 drivers/power/supply/abx500_chargalg.c|  2 +-
 drivers/power/supply/charger-manager.c|  2 +-
 drivers/rtc/rtc-pcf85063.c|  2 +-
 drivers/s390/scsi/zfcp_fsf.c  |  2 +-
 drivers/scsi/aic7xxx/aic79xx_core.c   |  4 ++--
 drivers/scsi/aic94xx/aic94xx_tmf.c|  2 +-
 drivers/scsi/lpfc/lpfc_sli.c  |  2 +-
 drivers/scsi/smartpqi/smartpqi_init.c |  2 +-
 drivers/scsi/sr.c |  2 +-
 drivers/tty/serial/sunsu.c|  2 +-
 drivers/tty/serial/sunzilog.c |  2 +-
 drivers/tty/vt/vt_ioctl.c |  2 +-
 drivers/usb/dwc3/core.c   |  2 +-
 drivers/usb/gadget/legacy/inode.c |  2 +-
 drivers/usb/gadget/udc/pxa25x_udc.c   |  4 ++--
 drivers/usb/host/ohci-hcd.c   |  2 +-
 drivers/usb/isp1760/isp1760-hcd.c |  2 +-
 drivers/usb/musb/

[Possible PATCH] iommu/qcom: Change CONFIG_BIG_ENDIAN to CONFIG_CPU_BIG_ENDIAN

[Joe Perches]

CONFIG_BIG_ENDIAN does not exist as a Kconfig symbol.

Signed-off-by: Joe Perches 
---

I don't have the hardware, so I can't tell if this is a
correct change, but it is a logical one.

Found by a test script that looks for IS_ENABLED(FOO)
where FOO must also exist in Kconfig files.

 drivers/iommu/qcom_iommu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/qcom_iommu.c b/drivers/iommu/qcom_iommu.c
index c3e1fbd1988c..69e113471ecb 100644
--- a/drivers/iommu/qcom_iommu.c
+++ b/drivers/iommu/qcom_iommu.c
@@ -304,7 +304,7 @@ static int qcom_iommu_init_domain(struct iommu_domain 
*domain,
  ARM_SMMU_SCTLR_M | ARM_SMMU_SCTLR_S1_ASIDPNE |
  ARM_SMMU_SCTLR_CFCFG;
 
-   if (IS_ENABLED(CONFIG_BIG_ENDIAN))
+   if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
reg |= ARM_SMMU_SCTLR_E;
 
iommu_writel(ctx, ARM_SMMU_CB_SCTLR, reg);

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 5.6 61/73] iommu/vt-d: Use right Kconfig option name

[Joe Perches]

On Mon, 2020-05-04 at 19:58 +0200, Greg Kroah-Hartman wrote:
> From: Lu Baolu 
> 
> commit ba61c3da00f4a5bf8805aeca1ba5ac3c9bd82e96 upstream.
> 
> The CONFIG_ prefix should be added in the code.
> 
> Fixes: 046182525db61 ("iommu/vt-d: Add Kconfig option to enable/disable 
> scalable mode")
> Reported-and-tested-by: Kumar, Sanjay K 
> Signed-off-by: Lu Baolu 
> Cc: Ashok Raj 
> Link: 
> https://lore.kernel.org/r/20200501072427.14265-1-baolu...@linux.intel.com
> Signed-off-by: Joerg Roedel 
> Signed-off-by: Greg Kroah-Hartman 
> 
> ---
>  drivers/iommu/intel-iommu.c |4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -371,11 +371,11 @@ int dmar_disabled = 0;
>  int dmar_disabled = 1;
>  #endif /* CONFIG_INTEL_IOMMU_DEFAULT_ON */
>  
> -#ifdef INTEL_IOMMU_SCALABLE_MODE_DEFAULT_ON
> +#ifdef CONFIG_INTEL_IOMMU_SCALABLE_MODE_DEFAULT_ON
>  int intel_iommu_sm = 1;
>  #else
>  int intel_iommu_sm;
> -#endif /* INTEL_IOMMU_SCALABLE_MODE_DEFAULT_ON */
> +#endif /* CONFIG_INTEL_IOMMU_SCALABLE_MODE_DEFAULT_ON */

Perhaps simpler as

int intel_iommu_sm = IS_BUILTIN(CONFIG_INTEL_IOMMU_SCALABLE_MODE_DEFAULT_ON);

So perhaps:
---
 drivers/iommu/intel-iommu.c | 13 ++---
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 0182cff2c7ac..ab8552c48391 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -365,17 +365,8 @@ static int intel_iommu_attach_device(struct iommu_domain 
*domain,
 static phys_addr_t intel_iommu_iova_to_phys(struct iommu_domain *domain,
dma_addr_t iova);
 
-#ifdef CONFIG_INTEL_IOMMU_DEFAULT_ON
-int dmar_disabled = 0;
-#else
-int dmar_disabled = 1;
-#endif /* CONFIG_INTEL_IOMMU_DEFAULT_ON */
-
-#ifdef CONFIG_INTEL_IOMMU_SCALABLE_MODE_DEFAULT_ON
-int intel_iommu_sm = 1;
-#else
-int intel_iommu_sm;
-#endif /* CONFIG_INTEL_IOMMU_SCALABLE_MODE_DEFAULT_ON */
+int dmar_disabled = !IS_BUILTIN(CONFIG_INTEL_IOMMU_DEFAULT_ON);
+int intel_iommu_sm = IS_BUILTIN(CONFIG_INTEL_IOMMU_SCALABLE_MODE_DEFAULT_ON);
 
 int intel_iommu_enabled = 0;
 EXPORT_SYMBOL_GPL(intel_iommu_enabled);

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] treewide: Rename "unencrypted" to "decrypted"

[Joe Perches]

On Tue, 2020-03-17 at 14:24 -0700, Dave Hansen wrote:
> On 3/17/20 2:06 PM, Borislav Petkov wrote:
> > On Tue, Mar 17, 2020 at 01:35:12PM -0700, Dave Hansen wrote:
> > > On 3/17/20 4:18 AM, Borislav Petkov wrote:
> > > > Back then when the whole SME machinery started getting mainlined, it
> > > > was agreed that for simplicity, clarity and sanity's sake, the terms
> > > > denoting encrypted and not-encrypted memory should be "encrypted" and
> > > > "decrypted". And the majority of the code sticks to that convention
> > > > except those two. So rename them.
> > > Don't "unencrypted" and "decrypted" mean different things?
> > > 
> > > Unencrypted to me means "encryption was never used for this data".
> > > 
> > > Decrypted means "this was/is encrypted but here is a plaintext copy".
> > Maybe but linguistical semantics is not the point here.
> > 
> > The idea is to represent a "binary" concept of memory being encrypted
> > or memory being not encrypted. And at the time we decided to use
> > "encrypted" and "decrypted" for those two things.
> 
> Yeah, agreed.  We're basically trying to name "!encrypted".
> 
> > Do you see the need to differentiate a third "state", so to speak, of
> > memory which was never encrypted?
> 
> No, there are just two states.  I just think the "!encrypted" case
> should not be called "decrypted".

Nor do I, it's completely misleading.


___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH -next 000/491] treewide: use fallthrough;

[Joe Perches]

There is a new fallthrough pseudo-keyword macro that can be used
to replace the various /* fallthrough */ style comments that are
used to indicate a case label code block is intended to fallthrough
to the next case label block.

See commit 294f69e662d1 ("compiler_attributes.h: Add 'fallthrough'
pseudo keyword for switch/case use")

These patches are intended to allow clang to detect missing
switch/case fallthrough uses.

Do a depth-first pass on the MAINTAINERS file and find the various
F: pattern files and convert the fallthrough comments to fallthrough;
for all files matched by all  F: patterns in in each section.

Done via the perl script below and the previously posted
cvt_fallthrough.pl script.

Link: 
https://lore.kernel.org/lkml/b56602fcf79f849e733e7b521bb0e17895d390fa.1582230379.git.joe.com/

These patches are based on next-20200310 and are available in

git://repo.or.cz/linux-2.6/trivial-mods.git in branch 20200310_fallthrough_2

$ cat commit_fallthrough.pl
#!/usr/bin/env perl

use sort 'stable';

#
# Reorder a sorted array so file entries are before directory entries
# depends on a trailing / for directories
# so:
#   foo/
#   foo/bar.c
# becomes
#   foo/bar.c
#   foo/
#
sub file_before_directory {
my ($array_ref) = (@_);

my $count = scalar(@$array_ref);

for (my $i = 1; $i < $count; $i++) {
if (substr(@$array_ref[$i - 1], -1) eq '/' &&
substr(@$array_ref[$i], 0, length(@$array_ref[$i - 1])) eq 
@$array_ref[$i - 1]) {
my $string = @$array_ref[$i - 1];
@$array_ref[$i - 1] = @$array_ref[$i];
@$array_ref[$i] = $string;
}
}
}

sub uniq {
my (@parms) = @_;

my %saw;
@parms = grep(!$saw{$_}++, @parms);

return @parms;
}

# Get all the F: file patterns in MAINTAINERS that could be a .[ch] file
my $maintainer_patterns = `grep -P '^F:\\s+' MAINTAINERS`;
my @patterns = split('\n', $maintainer_patterns);
s/^F:\s*// for @patterns;
@patterns = grep(!/^(?:Documentation|tools|scripts)\//, @patterns);
@patterns = grep(!/\.(?:dtsi?|rst|config)$/, @patterns);
@patterns = sort @patterns;
@patterns = sort { $b =~ tr/\//\// cmp $a =~ tr/\//\// } @patterns;
file_before_directory(\@patterns);

my %sections_done;

foreach my $pattern (@patterns) {

# Find the files the pattern matches
my $pattern_files = `git ls-files -- $pattern`;
my @new_patterns = split('\n', $pattern_files);
$pattern_files = join(' ', @new_patterns);
next if ($pattern_files =~ /^\s*$/);

# Find the section the first file matches
my $pattern_file = @new_patterns[0];
my $section_output = `./scripts/get_maintainer.pl --nogit --nogit-fallback 
--sections --pattern-depth=1 $pattern_file`;
my @section = split('\n', $section_output);
my $section_header = @section[0];

print("Section: <$section_header>\n");

# Skip the section if it's already done
print("Already done '$section_header'\n") if 
($sections_done{$section_header});
next if ($sections_done{$section_header}++);

# Find all the .[ch] files in all F: lines in that section
my @new_section;
foreach my $line (@section) {
last if ($line =~ /^\s*$/);
push(@new_section, $line);
}
@section = grep(/^F:/, @new_section);
s/^F:\s*// for @section;

@section = grep(!/^(?:Documentation|tools|scripts)\//, @section);
@section = grep(!/\.(?:dtsi?|rst|config)$/, @section);
@section = sort @section;
@section = uniq(@section);

my $section_files = join(' ', @section);

print("section_files: <$section_files>\n");

next if ($section_files =~ /^\s*$/);

my $cvt_files = `git ls-files -- $section_files`;
my @files = split('\n', $cvt_files);

@files = grep(!/^(?:Documentation|tools|scripts)\//, @files);
@files = grep(!/\.(?:dtsi?|rst|config)$/, @files);
@files = grep(/\.[ch]$/, @files);
@files = sort @files;
@files = uniq(@files);

$cvt_files = join(' ', @files);
print("files: <$cvt_files>\n");

next if (scalar(@files) < 1);

# Convert fallthroughs for all [.ch] files in the section
print("doing cvt_fallthrough.pl -- $cvt_files\n");

`cvt_fallthrough.pl -- $cvt_files`;

# If nothing changed, nothing to commit
`git diff-index --quiet HEAD --`;
next if (!$?);

# Commit the changes
my $fh;

open($fh, "+>", "cvt_fallthrough.commit_msg") or die "$0: can't create 
temporary file: $!\n";
print $fh <https://lore.kernel.org/lkml/b56602fcf79f849e733e7b521bb0e17895d390fa.1582230379.git@perches.com/
EOF
;
close $fh;

`git commit -s -a -F cvt_fallthrough.commit_msg`;
}

Joe Perches (491):
  MELLANOX ETHERNET INNOVA DRIVERS: Use fallthrough;
  MARVELL OCTEONTX2 RVU ADMIN FUNCTION DRIVER: Use fallthrough;
  MELLANOX MLX5 core VPI driver: Use fallthrough;
  PERFORMANCE EVENTS SUBSYSTEM: Use fallthrough;
  ARM/UNI

Re: [PATCH v5 8/8] iommu/vt-d: Misc macro clean up for SVM

[Joe Perches]

On Mon, 2019-12-02 at 11:58 -0800, Jacob Pan wrote:
> Use combined macros for_each_svm_dev() to simplify SVM device iteration
> and error checking.
[]
> diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
[]
> @@ -427,40 +430,36 @@ int intel_svm_unbind_mm(struct device *dev, int pasid)
[]
> + for_each_svm_dev(sdev, svm, dev) {
> + ret = 0;
> + sdev->users--;
> + if (!sdev->users) {

This might be better by reducing indentation here too

if (sdev->users)
break;

> + list_del_rcu(>list);

to reduce indentation 1 level below this

> + /* Flush the PASID cache and IOTLB for this device.
> +  * Note that we do depend on the hardware *not* using
> +  * the PASID any more. Just as we depend on other
> +  * devices never using PASIDs that they have no right
> +  * to use. We have a *shared* PASID table, because it's
> +  * large and has to be physically contiguous. So it's
> +  * hard to be as defensive as we might like. */
> + intel_pasid_tear_down_entry(iommu, dev, svm->pasid);
> + intel_flush_svm_range_dev(svm, sdev, 0, -1, 0);
> + kfree_rcu(sdev, rcu);
> +
> + if (list_empty(>devs)) {
> + ioasid_free(svm->pasid);
> + if (svm->mm)
> + mmu_notifier_unregister(>notifier, 
> svm->mm);
> + list_del(>list);
> + /* We mandate that no page faults may be 
> outstanding
> +  * for the PASID when intel_svm_unbind_mm() is 
> called.
> +  * If that is not obeyed, subtle errors will 
> happen.
> +  * Let's make them less subtle... */
> + memset(svm, 0x6b, sizeof(*svm));
> + kfree(svm);
>   }
> - break;
>   }
> + break;
>   }
>   out:
>   mutex_unlock(_mutex);

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 8/8] iommu/vt-d: Misc macro clean up for SVM

[Joe Perches]

On Mon, 2019-12-02 at 10:15 -0800, Jacob Pan wrote:
> On Thu, 21 Nov 2019 13:37:10 -0800
> Joe Perches  wrote:
> 
> > On Thu, 2019-11-21 at 13:26 -0800, Jacob Pan wrote:
> > > Use combined macros for_each_svm_dev() to simplify SVM device
> > > iteration and error checking.  
> > []
> > > diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c  
> > []
> > > +#define for_each_svm_dev(sdev, svm, d)   \
> > > + list_for_each_entry((sdev), &(svm)->devs, list) \
> > > + if ((d) != (sdev)->dev) {} else
> > > +
> > >  int intel_svm_bind_mm(struct device *dev, int *pasid, int flags,
> > > struct svm_dev_ops *ops) {
> > >   struct intel_iommu *iommu = intel_svm_device_to_iommu(dev);
> > > @@ -274,15 +278,13 @@ int intel_svm_bind_mm(struct device *dev, int
> > > *pasid, int flags, struct svm_dev_ goto out;
> > >   }
> > >  
> > > - list_for_each_entry(sdev, >devs,
> > > list) {
> > > - if (dev == sdev->dev) {
> > > - if (sdev->ops != ops) {
> > > - ret = -EBUSY;
> > > - goto out;
> > > - }
> > > - sdev->users++;
> > > - goto success;
> > > + for_each_svm_dev(sdev, svm, dev) {
> > > + if (sdev->ops != ops) {
> > > + ret = -EBUSY;
> > > + goto out;
> > >   }
> > > + sdev->users++;
> > > + goto success;
> > >   }  
> > 
> > I think this does not read better as this is now a
> > for_each loop that exits the loop on the first match.
> > 
> I think one of the benefits is reduced indentation. What do you
> recommend?

Making the code intelligible for a reader.

At least add a comment describing why there is only
a single possible match.

Given the for_each name, it's odd code that only the
first match has an action.


___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2] iommu/iova: silence warnings under memory pressure

[Joe Perches]

On Fri, 2019-11-22 at 11:46 -0500, Qian Cai wrote:
> On Fri, 2019-11-22 at 08:28 -0800, Joe Perches wrote:
> > On Fri, 2019-11-22 at 09:59 -0500, Qian Cai wrote:
> > > On Thu, 2019-11-21 at 20:37 -0800, Joe Perches wrote:
> > > > On Thu, 2019-11-21 at 21:55 -0500, Qian Cai wrote:
> > > > > When running heavy memory pressure workloads, this 5+ old system is
> > > > > throwing endless warnings below because disk IO is too slow to recover
> > > > > from swapping. Since the volume from alloc_iova_fast() could be large,
> > > > > once it calls printk(), it will trigger disk IO (writing to the log
> > > > > files) and pending softirqs which could cause an infinite loop and 
> > > > > make
> > > > > no progress for days by the ongoimng memory reclaim. This is the 
> > > > > counter
> > > > > part for Intel where the AMD part has already been merged. See the
> > > > > commit 3d708895325b ("iommu/amd: Silence warnings under memory
> > > > > pressure"). Since the allocation failure will be reported in
> > > > > intel_alloc_iova(), so just call printk_ratelimted() there and silence
> > > > > the one in alloc_iova_mem() to avoid the expensive warn_alloc().
> > > > 
> > > > []
> > > > > v2: use dev_err_ratelimited() and improve the commit messages.
> > > > 
> > > > []
> > > > > diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> > > > 
> > > > []
> > > > > @@ -3401,7 +3401,8 @@ static unsigned long intel_alloc_iova(struct 
> > > > > device *dev,
> > > > >   iova_pfn = alloc_iova_fast(>iovad, nrpages,
> > > > >  IOVA_PFN(dma_mask), true);
> > > > >   if (unlikely(!iova_pfn)) {
> > > > > - dev_err(dev, "Allocating %ld-page iova failed", 
> > > > > nrpages);
> > > > > + dev_err_ratelimited(dev, "Allocating %ld-page iova 
> > > > > failed",
> > > > > + nrpages);
> > > > 
> > > > Trivia:
> > > > 
> > > > This should really have a \n termination on the format string
> > > > 
> > > > dev_err_ratelimited(dev, "Allocating %ld-page iova 
> > > > failed\n",
> > > > 
> > > > 
> > > 
> > > Why do you say so? It is right now printing with a newline added anyway.
> > > 
> > >  hpsa :03:00.0: DMAR: Allocating 1-page iova failed
> > 
> > If another process uses pr_cont at the same time,
> > it can be interleaved.
> 
> I lean towards fixing that in a separate patch if ever needed, as the origin
> dev_err() has no "\n" enclosed either.

Your choice.

I wrote trivia:, but touching the same line multiple times
is relatively pointless.



___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2] iommu/iova: silence warnings under memory pressure

[Joe Perches]

On Fri, 2019-11-22 at 09:59 -0500, Qian Cai wrote:
> On Thu, 2019-11-21 at 20:37 -0800, Joe Perches wrote:
> > On Thu, 2019-11-21 at 21:55 -0500, Qian Cai wrote:
> > > When running heavy memory pressure workloads, this 5+ old system is
> > > throwing endless warnings below because disk IO is too slow to recover
> > > from swapping. Since the volume from alloc_iova_fast() could be large,
> > > once it calls printk(), it will trigger disk IO (writing to the log
> > > files) and pending softirqs which could cause an infinite loop and make
> > > no progress for days by the ongoimng memory reclaim. This is the counter
> > > part for Intel where the AMD part has already been merged. See the
> > > commit 3d708895325b ("iommu/amd: Silence warnings under memory
> > > pressure"). Since the allocation failure will be reported in
> > > intel_alloc_iova(), so just call printk_ratelimted() there and silence
> > > the one in alloc_iova_mem() to avoid the expensive warn_alloc().
> > 
> > []
> > > v2: use dev_err_ratelimited() and improve the commit messages.
> > 
> > []
> > > diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> > 
> > []
> > > @@ -3401,7 +3401,8 @@ static unsigned long intel_alloc_iova(struct device 
> > > *dev,
> > >   iova_pfn = alloc_iova_fast(>iovad, nrpages,
> > >  IOVA_PFN(dma_mask), true);
> > >   if (unlikely(!iova_pfn)) {
> > > - dev_err(dev, "Allocating %ld-page iova failed", nrpages);
> > > + dev_err_ratelimited(dev, "Allocating %ld-page iova failed",
> > > + nrpages);
> > 
> > Trivia:
> > 
> > This should really have a \n termination on the format string
> > 
> > dev_err_ratelimited(dev, "Allocating %ld-page iova failed\n",
> > 
> > 
> 
> Why do you say so? It is right now printing with a newline added anyway.
> 
>  hpsa :03:00.0: DMAR: Allocating 1-page iova failed

If another process uses pr_cont at the same time,
it can be interleaved.


___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2] iommu/iova: silence warnings under memory pressure

[Joe Perches]

On Thu, 2019-11-21 at 21:55 -0500, Qian Cai wrote:
> When running heavy memory pressure workloads, this 5+ old system is
> throwing endless warnings below because disk IO is too slow to recover
> from swapping. Since the volume from alloc_iova_fast() could be large,
> once it calls printk(), it will trigger disk IO (writing to the log
> files) and pending softirqs which could cause an infinite loop and make
> no progress for days by the ongoimng memory reclaim. This is the counter
> part for Intel where the AMD part has already been merged. See the
> commit 3d708895325b ("iommu/amd: Silence warnings under memory
> pressure"). Since the allocation failure will be reported in
> intel_alloc_iova(), so just call printk_ratelimted() there and silence
> the one in alloc_iova_mem() to avoid the expensive warn_alloc().
[]
> v2: use dev_err_ratelimited() and improve the commit messages.
[]
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
[]
> @@ -3401,7 +3401,8 @@ static unsigned long intel_alloc_iova(struct device 
> *dev,
>   iova_pfn = alloc_iova_fast(>iovad, nrpages,
>  IOVA_PFN(dma_mask), true);
>   if (unlikely(!iova_pfn)) {
> - dev_err(dev, "Allocating %ld-page iova failed", nrpages);
> + dev_err_ratelimited(dev, "Allocating %ld-page iova failed",
> + nrpages);

Trivia:

This should really have a \n termination on the format string

dev_err_ratelimited(dev, "Allocating %ld-page iova failed\n",


___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 8/8] iommu/vt-d: Misc macro clean up for SVM

[Joe Perches]

On Thu, 2019-11-21 at 13:26 -0800, Jacob Pan wrote:
> Use combined macros for_each_svm_dev() to simplify SVM device iteration
> and error checking.
[]
> diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
[]
> +#define for_each_svm_dev(sdev, svm, d)   \
> + list_for_each_entry((sdev), &(svm)->devs, list) \
> + if ((d) != (sdev)->dev) {} else
> +
>  int intel_svm_bind_mm(struct device *dev, int *pasid, int flags, struct 
> svm_dev_ops *ops)
>  {
>   struct intel_iommu *iommu = intel_svm_device_to_iommu(dev);
> @@ -274,15 +278,13 @@ int intel_svm_bind_mm(struct device *dev, int *pasid, 
> int flags, struct svm_dev_
>   goto out;
>   }
>  
> - list_for_each_entry(sdev, >devs, list) {
> - if (dev == sdev->dev) {
> - if (sdev->ops != ops) {
> - ret = -EBUSY;
> - goto out;
> - }
> - sdev->users++;
> - goto success;
> + for_each_svm_dev(sdev, svm, dev) {
> + if (sdev->ops != ops) {
> + ret = -EBUSY;
> + goto out;
>   }
> + sdev->users++;
> + goto success;
>   }

I think this does not read better as this is now a
for_each loop that exits the loop on the first match.

>  
>   break;
> @@ -427,43 +429,36 @@ int intel_svm_unbind_mm(struct device *dev, int pasid)
>   goto out;
>   }
>  
> - if (!svm)
> - goto out;
> -
> - list_for_each_entry(sdev, >devs, list) {
[]
> + for_each_svm_dev(sdev, svm, dev) {

I think this should not remove the !svm test above.


___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 1/4] dma-debug: Use pr_fmt()

[Joe Perches]

On Mon, 2018-12-03 at 17:28 +, Robin Murphy wrote:
> Use pr_fmt() to generate the "DMA-API: " prefix consistently. This
> results in it being added to a couple of pr_*() messages which were
> missing it before, and for the err_printk() calls moves it to the actual
> start of the message instead of somewhere in the middle.
> 
> Signed-off-by: Robin Murphy 
> ---
> 
> I chose not to refactor the existing split strings for minimal churn here.
> 
>  kernel/dma/debug.c | 74 --
>  1 file changed, 38 insertions(+), 36 deletions(-)
> 
> diff --git a/kernel/dma/debug.c b/kernel/dma/debug.c
> index 231ca4628062..91b84140e4a5 100644
> --- a/kernel/dma/debug.c
> +++ b/kernel/dma/debug.c
> @@ -17,6 +17,8 @@
>   * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307 USA
>   */
>  
> +#define pr_fmt(fmt)  "DMA-API: " fmt
> +
>  #include 
>  #include 
>  #include 
> @@ -234,7 +236,7 @@ static bool driver_filter(struct device *dev)
>   error_count += 1;   \
>   if (driver_filter(dev) &&   \
>   (show_all_errors || show_num_errors > 0)) { \
> - WARN(1, "%s %s: " format,   \
> + WARN(1, pr_fmt("%s %s: ") format,   \
>dev ? dev_driver_string(dev) : "NULL", \
>dev ? dev_name(dev) : "NULL", ## arg); \
>   dump_entry_trace(entry);\

I think converting this WARN to

dev_err(dev, format, ##__VA_ARGS__);
dump_stack();

would look better and be more intelligible.

Perhaps add a #define for dev_fmt if really necessary.


___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] iommu/amd: fix missing tag from dev_err message

[Joe Perches]

On Tue, 2018-07-03 at 18:57 +0200, Walter Harms wrote:
> It is only cosmetics but even the author got lost about the loose bracket.
> I would suggest to remove all the brackets or if needed to move the [ in the
> message. We have enought memory this days.

My suggestion would be to remove the separate dev_err
and use "Event log [" in each dev_err.

So here's an actual suggested patch that does a few things:

o Coalesce formats and realigns arguments
o Uses pr_fmt and dev_fmt to prefix "AMD-Vi: " to all messages
o Remove embedded "AMD-Vi: " from various formats

Some existing pr_ uses now are prefixed too.
---
 drivers/iommu/amd_iommu.c | 69 +++
 1 file changed, 34 insertions(+), 35 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 596b95c50051..01583a8ccaf9 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -17,6 +17,9 @@
  * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307 USA
  */
 
+#define pr_fmt(fmt) "AMD-Vi: " fmt
+#define dev_fmt pr_fmt
+
 #include 
 #include 
 #include 
@@ -273,9 +276,8 @@ static u16 get_alias(struct device *dev)
return pci_alias;
}
 
-   pr_info("AMD-Vi: Using IVRS reported alias %02x:%02x.%d "
-   "for device %s[%04x:%04x], kernel reported alias "
-   "%02x:%02x.%d\n", PCI_BUS_NUM(ivrs_alias), PCI_SLOT(ivrs_alias),
+   pr_info("Using IVRS reported alias %02x:%02x.%d for device 
%s[%04x:%04x], kernel reported alias %02x:%02x.%d\n",
+   PCI_BUS_NUM(ivrs_alias), PCI_SLOT(ivrs_alias),
PCI_FUNC(ivrs_alias), dev_name(dev), pdev->vendor, pdev->device,
PCI_BUS_NUM(pci_alias), PCI_SLOT(pci_alias),
PCI_FUNC(pci_alias));
@@ -287,7 +289,7 @@ static u16 get_alias(struct device *dev)
if (pci_alias == devid &&
PCI_BUS_NUM(ivrs_alias) == pdev->bus->number) {
pci_add_dma_alias(pdev, ivrs_alias & 0xff);
-   pr_info("AMD-Vi: Added PCI DMA alias %02x.%d for %s\n",
+   pr_info("Added PCI DMA alias %02x.%d for %s\n",
PCI_SLOT(ivrs_alias), PCI_FUNC(ivrs_alias),
dev_name(dev));
}
@@ -507,8 +509,8 @@ static void dump_dte_entry(u16 devid)
int i;
 
for (i = 0; i < 4; ++i)
-   pr_err("AMD-Vi: DTE[%d]: %016llx\n", i,
-   amd_iommu_dev_table[devid].data[i]);
+   pr_err("DTE[%d]: %016llx\n",
+  i, amd_iommu_dev_table[devid].data[i]);
 }
 
 static void dump_command(unsigned long phys_addr)
@@ -517,7 +519,7 @@ static void dump_command(unsigned long phys_addr)
int i;
 
for (i = 0; i < 4; ++i)
-   pr_err("AMD-Vi: CMD[%d]: %08x\n", i, cmd->data[i]);
+   pr_err("CMD[%d]: %08x\n", i, cmd->data[i]);
 }
 
 static void amd_iommu_report_page_fault(u16 devid, u16 domain_id,
@@ -532,10 +534,10 @@ static void amd_iommu_report_page_fault(u16 devid, u16 
domain_id,
dev_data = get_dev_data(>dev);
 
if (dev_data && __ratelimit(_data->rs)) {
-   dev_err(>dev, "AMD-Vi: Event logged [IO_PAGE_FAULT 
domain=0x%04x address=0x%016llx flags=0x%04x]\n",
+   dev_err(>dev, "Event logged [IO_PAGE_FAULT domain=0x%04x 
address=0x%016llx flags=0x%04x]\n",
domain_id, address, flags);
} else if (printk_ratelimit()) {
-   pr_err("AMD-Vi: Event logged [IO_PAGE_FAULT device=%02x:%02x.%x 
domain=0x%04x address=0x%016llx flags=0x%04x]\n",
+   pr_err("Event logged [IO_PAGE_FAULT device=%02x:%02x.%x 
domain=0x%04x address=0x%016llx flags=0x%04x]\n",
PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
domain_id, address, flags);
}
@@ -562,7 +564,7 @@ static void iommu_print_event(struct amd_iommu *iommu, void 
*__evt)
if (type == 0) {
/* Did we hit the erratum? */
if (++count == LOOP_TIMEOUT) {
-   pr_err("AMD-Vi: No event written to event log\n");
+   pr_err("No event written to event log\n");
return;
}
udelay(1);
@@ -572,43 +574,40 @@ static void iommu_print_event(struct amd_iommu *iommu, 
void *__evt)
if (type == EVENT_TYPE_IO_FAULT) {
amd_iommu_report_page_fault(devid, pasid, address, flags);
return;
-   } else {
-   dev_err(dev, "AMD-Vi: Event logged [");
}
 
switch (type) {
case EVENT_TYPE_ILL_DEV:
-   dev_err(dev, "ILLEGAL_DEV_TABLE_ENTRY device=%02x:%02x.%x 
pasid=0x%05x address=0x%016llx flags=0x%04x]\n",
+   dev_err(dev, "Event logged [ILLEGAL_DEV_TABLE_ENTRY 
device=%02x:%02x.%x pasid=0x%05x address=0x%016llx flags=0x%04x]\n",
PCI_BUS_NUM(devid), 

Re: [PATCH] iommu/amd: fix missing tag from dev_err message

[Joe Perches]

On Tue, 2018-07-03 at 07:56 -0500, Gary R Hook wrote:
> On 07/03/2018 05:07 AM, Joe Perches wrote:
> > On Tue, 2018-07-03 at 07:40 +0100, Colin King wrote:
> > > Currently tag is being assigned but not used, it is missing from
> > > the dev_err message, so add it in.
> > > 
> > > Cleans up clang warning:
> > > warning: variable 'tag' set but not used [-Wunused-but-set-variable]
> > 
> > []
> > > diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> > 
> > []
> > > @@ -616,9 +616,9 @@ static void iommu_print_event(struct amd_iommu 
> > > *iommu, void *__evt)
> > >   pasid = ((event[0] >> 16) & 0x)
> > >   | ((event[1] << 6) & 0xF);
> > >   tag = event[1] & 0x03FF;
> > > - dev_err(dev, "INVALID_PPR_REQUEST device=%02x:%02x.%x 
> > > pasid=0x%05x address=0x%016llx flags=0x%04x]\n",
> > > + dev_err(dev, "INVALID_PPR_REQUEST device=%02x:%02x.%x 
> > > pasid=0x%05x address=0x%016llx flags=0x%04x tag=0x%03x]\n",
> > >   PCI_BUS_NUM(devid), PCI_SLOT(devid), 
> > > PCI_FUNC(devid),
> > > - pasid, address, flags);
> > > + pasid, address, flags, tag);
> > 
> > Seems to have a superfluous ] that should be removed.
> 
> Yeah, I pretty much messed up all of the log messages in that function. 
> My apologies. I'll create a patch for that problem; it shouldn't be 
> fixed here.

I also wonder why event is declared volatile and then
dereferenced with [] multiple times.

Maybe each array dereference should be stored as a
local variable instead.


___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] iommu/amd: fix missing tag from dev_err message

[Joe Perches]

On Tue, 2018-07-03 at 11:27 -0500, Hook, Gary wrote:
> On 7/3/2018 11:24 AM, Colin Ian King wrote:
> > On 03/07/18 17:21, Hook, Gary wrote:
> > > On 7/3/2018 10:55 AM, Joe Perches wrote:
> > > > On Tue, 2018-07-03 at 07:56 -0500, Gary R Hook wrote:
> > > > > On 07/03/2018 05:07 AM, Joe Perches wrote:
> > > > > > On Tue, 2018-07-03 at 07:40 +0100, Colin King wrote:
> > > > > > > Currently tag is being assigned but not used, it is missing from
> > > > > > > the dev_err message, so add it in.
> > > > > > > 
> > > > > > > Cleans up clang warning:
> > > > > > > warning: variable 'tag' set but not used 
> > > > > > > [-Wunused-but-set-variable]
> > > > > > 
> > > > > > []
> > > > > > > diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> > > > > > 
> > > > > > []
> > > > > > > @@ -616,9 +616,9 @@ static void iommu_print_event(struct amd_iommu
> > > > > > > *iommu, void *__evt)
> > > > > > > pasid = ((event[0] >> 16) & 0x)
> > > > > > > | ((event[1] << 6) & 0xF);
> > > > > > > tag = event[1] & 0x03FF;
> > > > > > > -dev_err(dev, "INVALID_PPR_REQUEST device=%02x:%02x.%x
> > > > > > > pasid=0x%05x address=0x%016llx flags=0x%04x]\n",
> > > > > > > +dev_err(dev, "INVALID_PPR_REQUEST device=%02x:%02x.%x
> > > > > > > pasid=0x%05x address=0x%016llx flags=0x%04x tag=0x%03x]\n",
> > > > > > > PCI_BUS_NUM(devid), PCI_SLOT(devid), 
> > > > > > > PCI_FUNC(devid),
> > > > > > > -pasid, address, flags);
> > > > > > > +pasid, address, flags, tag);
> > > > > > 
> > > > > > Seems to have a superfluous ] that should be removed.
> > > > > 
> > > > > Yeah, I pretty much messed up all of the log messages in that 
> > > > > function.
> > > > > My apologies. I'll create a patch for that problem; it shouldn't be
> > > > > fixed here.
> > > 
> > > Well, no, I misremembered. The extraneous square brace has been there
> > > forever. Needs fixin', though.
> > > 
> > 
> > The opening square bracket is much earlier:
> > 
> > dev_err(dev, "AMD-Vi: Event logged [");
> > 
> > ..and all the subsequent dev_err messages have the trailing square bracket.
> 
> Gah! Mystery solved. I'd forgotten about that.
> 
> "Never mind."
> 

It stems from the misconversion from printk to dev_err

The initial dev_err is fine but all the dev_err uses
in the switch/case should use pr_cont, not dev_err

(there's a suggested patch below this commit reference)

---
>From 90ca3859f5ea90050d00e695355934b37357e7bb Mon Sep 17 00:00:00 2001
From: Gary R Hook 
Date: Thu, 8 Mar 2018 18:34:41 -0600
Subject: [PATCH] iommu/amd: Use dev_err to send events to the system log

Remove printk and use a more preferable error logging function.

Signed-off-by: Gary R Hook 
Signed-off-by: Joerg Roedel 
---
 drivers/iommu/amd_iommu.c | 55 
---
 1 file changed, 28 insertions(+), 27 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 997a947ddc3b..4cd19f93ca15 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -547,6 +547,7 @@ static void amd_iommu_report_page_fault(u16 devid, u16 
domain_id,
 
 static void iommu_print_event(struct amd_iommu *iommu, void *__evt)
 {
+   struct device *dev = iommu->iommu.dev;
int type, devid, domid, flags;
volatile u32 *event = __evt;
int count = 0;
@@ -573,53 +574,53 @@ static void iommu_print_event(struct amd_iommu *iommu, 
void *__evt)
amd_iommu_report_page_fault(devid, domid, address, flags);
return;
} else {
-   printk(KERN_ERR "AMD-Vi: Event logged [");
+   dev_err(dev, "AMD-Vi: Event logged [");
}
 
switch (type) {
case EVENT_TYPE_ILL_DEV:
-   printk("ILLEGAL_DEV_TABLE_ENTRY device=%02x:%02x.%x "
-  "address=0x%016llx flags=0x%04x]\n",
-  PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
-  address, flags);
+   dev_err(dev, "ILLEGAL_DEV_TABLE_ENTRY devic

Re: [PATCH] iommu/amd: fix missing tag from dev_err message

[Joe Perches]

On Tue, 2018-07-03 at 07:40 +0100, Colin King wrote:
> Currently tag is being assigned but not used, it is missing from
> the dev_err message, so add it in.
> 
> Cleans up clang warning:
> warning: variable 'tag' set but not used [-Wunused-but-set-variable]
[]
> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
[]
> @@ -616,9 +616,9 @@ static void iommu_print_event(struct amd_iommu *iommu, 
> void *__evt)
>   pasid = ((event[0] >> 16) & 0x)
>   | ((event[1] << 6) & 0xF);
>   tag = event[1] & 0x03FF;
> - dev_err(dev, "INVALID_PPR_REQUEST device=%02x:%02x.%x 
> pasid=0x%05x address=0x%016llx flags=0x%04x]\n",
> + dev_err(dev, "INVALID_PPR_REQUEST device=%02x:%02x.%x 
> pasid=0x%05x address=0x%016llx flags=0x%04x tag=0x%03x]\n",
>   PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
> - pasid, address, flags);
> + pasid, address, flags, tag);

Seems to have a superfluous ] that should be removed.

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH 00/18] Convert default pr_fmt from empty to KBUILD_MODNAME

[Joe Perches]

pr_ logging uses allow a prefix to be specified with a
specific #define pr_fmt

The default of pr_fmt in printk.h is #define pr_fmt(fmt) fmt
so no prefixing of logging output is generically done.

There are several output logging uses like dump_stack() that are
unprefixed and should remain unprefixed.

This patch series attempts to convert the default #define of pr_fmt to
KBUILD_MODNAME ": " fmt and as well update the various bits of the kernel
that should _not_ be prefixed by adding #define pr_fmt(fmt) fmt to those
compilation units that do not want output message prefixing.

There are about 1200 uses of #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
that could be removed if the default is changed.

A script that does this removal (and removes any blank lines that follow)
for the linux-kernel tree is included below:

$ git grep -P --name-only "define\s+pr_fmt\b" | \
  grep -v include/linux/printk.h | \
  xargs perl -i -e 'local $/; while (<>) {s/(\n)*[ \t]*#[ \t]*define[ 
\t]+pr_fmt[ \t]*\([ \t]*(\w+)[ \t]*\)[ \t]*KBUILD_MODNAME[ \t]*\": \"[ \t]*\2[ 
\t]*\n\s*/\1\n/; s/^\n//; print;}'

This script should be run after this patch series is applied.

The above script output diff is currently:

1198 files changed, 70 insertions(+), 2241 deletions(-)

Joe Perches (18):
  kernel: Use pr_fmt
  lib: Use pr_fmt
  printk: Convert pr_fmt from blank define to KBUILD_MODNAME
  x86: Remove pr_fmt duplicate logging prefixes
  x86/mtrr: Rename main.c to mtrr.c and remove duplicate prefixes
  net: Remove pr_fmt duplicate logging prefixes
  blk-mq: Remove pr_fmt duplicate logging prefixes
  random: Remove pr_fmt duplicate logging prefixes
  ptp: Remove pr_fmt duplicate logging prefixes
  efifb: Remove pr_fmt duplicate logging prefixes
  proc: Remove pr_fmt duplicate logging prefixes
  uprobes: Remove pr_fmt duplicate logging prefixes
  printk: Remove pr_fmt duplicate logging prefixes
  lib/mpi: Remove pr_fmt duplicate logging prefixes
  security: Remove pr_fmt duplicate logging prefixes
  aoe: Remove pr_fmt duplicate logging prefixes
  security: encrypted-keys: Remove pr_fmt duplicate logging prefixes
  rcu: Use pr_fmt to prefix "rcu: " to logging output

 arch/x86/events/amd/ibs.c  |  2 +-
 arch/x86/kernel/cpu/mtrr/Makefile  |  2 +-
 arch/x86/kernel/cpu/mtrr/{main.c => mtrr.c}| 33 ++---
 arch/x86/kernel/e820.c | 32 ++--
 arch/x86/kernel/hpet.c |  5 +-
 arch/x86/kernel/uprobes.c  |  4 +-
 arch/x86/mm/numa.c | 22 -
 block/blk-mq.c |  9 ++--
 drivers/block/aoe/aoeblk.c | 29 ++-
 drivers/block/aoe/aoechr.c | 11 ++---
 drivers/block/aoe/aoecmd.c | 34 ++---
 drivers/block/aoe/aoedev.c | 19 +++-
 drivers/block/aoe/aoemain.c|  6 +--
 drivers/block/aoe/aoenet.c | 19 +++-
 drivers/char/hw_random/via-rng.c   | 10 ++--
 drivers/char/random.c  | 16 +++---
 drivers/ptp/ptp_clock.c|  4 +-
 drivers/video/fbdev/efifb.c| 48 +-
 fs/proc/root.c |  6 +--
 include/linux/printk.h |  2 +-
 kernel/acct.c  |  2 +
 kernel/async.c | 14 +++---
 kernel/audit_tree.c|  2 +-
 kernel/backtracetest.c |  8 +--
 kernel/crash_core.c| 29 ++-
 kernel/events/uprobes.c|  3 +-
 kernel/exit.c  |  2 +
 kernel/hung_task.c | 13 +++--
 kernel/kprobes.c   | 20 +---
 kernel/module.c| 59 +++
 kernel/panic.c |  3 ++
 kernel/params.c| 13 +++--
 kernel/pid.c   |  2 +
 kernel/printk/printk.c |  2 +-
 kernel/profile.c   |  2 +
 kernel/range.c |  2 +-
 kernel/rcu/rcu_segcblist.c |  2 +
 kernel/rcu/rcuperf.c   | 10 ++--
 kernel/rcu/rcutorture.c| 46 +-
 kernel/rcu/srcutiny.c  |  2 +
 kernel/rcu/srcutree.c  |  5 +-
 kernel/rcu/tiny.c  |  3 ++
 kernel/rcu/tree.c  |  8 +--
 kernel/rcu/tree_plugin.h   | 67 +++---
 kernel/rcu/update.c| 19 +---
 kernel/relay.c |  5 +-
 ker

[PATCH 02/18] lib: Use pr_fmt

[Joe Perches]

Sometime in the future, it would be useful to convert pr_fmt from a
default simple define to use a default prefix with KBUILD_MODNAME.

There are files in lib/ that use pr_, some with an embedded
prefix, that also do not have a specific pr_fmt define.

Add pr_fmt for those files.

There are some differences in output as some logging output now is
prefixed with the appropriate KBUILD_MODNAME.

Miscellanea:

o Simplify debugging macros to only require a single function definition
  by moving #define DEBUG into the definition and removing the empty
  function definition
o Function alignment neatening
o Use %s, __func__ instead of embedding function names as strings
o Add missing newline terminations

Signed-off-by: Joe Perches <j...@perches.com>
---
 lib/cpu_rmap.c   | 15 +---
 lib/crc32test.c  |  2 ++
 lib/earlycpio.c  |  5 ++--
 lib/find_bit_benchmark.c |  2 ++
 lib/kobject.c| 36 ++--
 lib/kobject_uevent.c | 27 ++---
 lib/nmi_backtrace.c  |  3 +++
 lib/percpu_ida.c |  4 +++-
 lib/percpu_test.c|  2 ++
 lib/random32.c   | 10 
 lib/stmp_device.c|  2 ++
 lib/string.c |  2 ++
 lib/swiotlb.c|  4 +++-
 lib/test_debug_virtual.c |  2 ++
 lib/test_rhashtable.c| 44 ++
 lib/test_sort.c  |  2 ++
 lib/ubsan.c  | 61 
 17 files changed, 122 insertions(+), 101 deletions(-)

diff --git a/lib/cpu_rmap.c b/lib/cpu_rmap.c
index f610b2a10b3e..2d7204928c60 100644
--- a/lib/cpu_rmap.c
+++ b/lib/cpu_rmap.c
@@ -7,6 +7,8 @@
  * by the Free Software Foundation, incorporated herein by reference.
  */
 
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #include 
 #include 
 #include 
@@ -110,26 +112,21 @@ static bool cpu_rmap_copy_neigh(struct cpu_rmap *rmap, 
unsigned int cpu,
return false;
 }
 
-#ifdef DEBUG
 static void debug_print_rmap(const struct cpu_rmap *rmap, const char *prefix)
 {
+#ifdef DEBUG
unsigned index;
unsigned int cpu;
 
-   pr_info("cpu_rmap %p, %s:\n", rmap, prefix);
+   pr_info("%p, %s:\n", rmap, prefix);
 
for_each_possible_cpu(cpu) {
index = rmap->near[cpu].index;
pr_info("cpu %d -> obj %u (distance %u)\n",
cpu, index, rmap->near[cpu].dist);
}
-}
-#else
-static inline void
-debug_print_rmap(const struct cpu_rmap *rmap, const char *prefix)
-{
-}
 #endif
+}
 
 /**
  * cpu_rmap_add - add object to a rmap
@@ -258,7 +255,7 @@ irq_cpu_rmap_notify(struct irq_affinity_notify *notify, 
const cpumask_t *mask)
 
rc = cpu_rmap_update(glue->rmap, glue->index, mask);
if (rc)
-   pr_warning("irq_cpu_rmap_notify: update failed: %d\n", rc);
+   pr_warn("%s: update failed: %d\n", __func__, rc);
 }
 
 /**
diff --git a/lib/crc32test.c b/lib/crc32test.c
index 97d6a57cefcc..63bb08ccb6f3 100644
--- a/lib/crc32test.c
+++ b/lib/crc32test.c
@@ -24,6 +24,8 @@
  * Version 2.  See the file COPYING for more details.
  */
 
+#define pr_fmt(fmt) fmt
+
 #include 
 #include 
 #include 
diff --git a/lib/earlycpio.c b/lib/earlycpio.c
index db283ba4d2c1..e98816b13719 100644
--- a/lib/earlycpio.c
+++ b/lib/earlycpio.c
@@ -130,9 +130,8 @@ struct cpio_data find_cpio_data(const char *path, void 
*data,
*nextoff = (long)nptr - (long)data;
 
if (ch[C_NAMESIZE] - mypathsize >= MAX_CPIO_FILE_NAME) {
-   pr_warn(
-   "File %s exceeding MAX_CPIO_FILE_NAME [%d]\n",
-   p, MAX_CPIO_FILE_NAME);
+   pr_warn("File %s exceeding MAX_CPIO_FILE_NAME 
[%d]\n",
+   p, MAX_CPIO_FILE_NAME);
}
strlcpy(cd.name, p + mypathsize, MAX_CPIO_FILE_NAME);
 
diff --git a/lib/find_bit_benchmark.c b/lib/find_bit_benchmark.c
index 5367ffa5c18f..d59d92c9e73e 100644
--- a/lib/find_bit_benchmark.c
+++ b/lib/find_bit_benchmark.c
@@ -24,6 +24,8 @@
  * - sparse bitmap with few set bits at random positions.
  */
 
+#define pr_fmt(fmt) fmt
+
 #include 
 #include 
 #include 
diff --git a/lib/kobject.c b/lib/kobject.c
index 18989b5b3b56..2bb9a50da4b0 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -10,6 +10,8 @@
  * about using the kobject interface.
  */
 
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #include 
 #include 
 #include 
@@ -129,8 +131,8 @@ static void fill_kobj_path(struct kobject *kobj, char 
*path, int length)
*(path + --length) = '/';
}
 
-   pr_debug("kobject: '%s' (%p): %s: path = '%s'\n", kobject_name(kobj),
-kobj, __func__, path);
+   pr_debug("'%

Re: [PATCH v5 1/2] iommu - Enable debugfs exposure of IOMMU driver internals

[Joe Perches]

On Tue, 2018-05-08 at 15:07 -0500, Gary R Hook wrote:
> On 05/08/2018 01:48 PM, Joe Perches wrote:
> > On Tue, 2018-05-08 at 12:08 -0500, Hook, Gary wrote:
> > > On 5/7/2018 6:47 PM, kbuild test robot wrote:
> > > > 
> > > > All error/warnings (new ones prefixed by >>):
> > > > 
> > > >  In file included from include/linux/intel-iommu.h:32:0,
> > > >   from drivers/gpu/drm/i915/i915_drv.h:41,
> > > >   from drivers/gpu/drm/i915/i915_oa_bxt.c:31:
> > > >  include/linux/iommu.h: In function 'iommu_debugfs_new_driver_dir':
> > > > > > include/linux/iommu.h:706:8: error: parameter name omitted
> > > > 
> > > >   struct dentry *iommu_debugfs_new_driver_dir(char *) {};
> > > >  ^~
> > > >  In file included from include/linux/intel-iommu.h:32:0,
> > > >   from drivers/gpu/drm/i915/i915_drv.h:41,
> > > >   from drivers/gpu/drm/i915/i915_oa_bxt.c:31:
> > > > > > include/linux/iommu.h:706:8: warning: control reaches end of 
> > > > > > non-void function [-Wreturn-type]
> > > > 
> > > >   struct dentry *iommu_debugfs_new_driver_dir(char *) {};
> > > >  ^~
> > > > 
> > > > vim +706 include/linux/iommu.h
> > > > 
> > > >  700
> > > >  701#ifdef CONFIG_IOMMU_DEBUGFS
> > > >  702void iommu_debugfs_setup(void);
> > > >  703struct dentry *iommu_debugfs_new_driver_dir(char *);
> > > >  704#else
> > > >  705static inline void iommu_debugfs_setup(void) {}
> > > >> 706struct dentry *iommu_debugfs_new_driver_dir(char *) {};
> > > >  707#endif
> > > >  708
> > > 
> > > I have no problems with adding parameter names. But
> > > scripts/checkpatch.pl doesn't seem to check for this, nor require it.
> > > Should checkpatch be updated?
> > 
> > I'm pretty sure that's not feasible.
> 
> Ugh. This is a definition, not a declaration. My bad. Which is likely 
> why I decided to apologize up front.
> 
> > And when the compiler tells you you've stuffed up some
> > syntactical bit, why should checkpatch duplicate the
> > output error message too?
> 
> Well, that's the point: neither the 4.8 nor 5.4 compiler complained 
> about this.

Perhaps because CONFIG_IOMMU_DEBUGFS was set in the .config
for all the compilation previously performed?

>  Not as an error, despite the fact that (now that I read what 
> is actually here, as opposed to what I think is there) this is wrong. 
> Had an error message been emitted, and the make stopped, I would have 
> figure this out before embarrassing myself in front of the entire interwebs.

There's no reason for that figuring out to be necessary.
Linux compilation complexity is pretty high and almost
no one understands it completely.

cheers, Joe
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v5 1/2] iommu - Enable debugfs exposure of IOMMU driver internals

[Joe Perches]

On Tue, 2018-05-08 at 12:08 -0500, Hook, Gary wrote:
> On 5/7/2018 6:47 PM, kbuild test robot wrote:
> > Hi Gary,
> > 
> 
> I imagine these questions have been asked before, so I'll ask for
> forgiveness up front.
> 
> 
> > Thank you for the patch! Yet something to improve:
> > 
> > [auto build test ERROR on iommu/next]
> > [also build test ERROR on v4.17-rc4 next-20180507]
> > [if your patch is applied to the wrong git tree, please drop us a note to 
> > help improve the system]
> > 
> > url:
> > https://github.com/0day-ci/linux/commits/Gary-R-Hook/iommu-Enable-debugfs-exposure-of-IOMMU-driver-internals/20180508-062918
> > base:   https://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git next
> > config: x86_64-randconfig-x016-201818 (attached as .config)
> > compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
> > reproduce:
> >  # save the attached .config to linux build tree
> >  make ARCH=x86_64
> 
> Is the gcc 7 compiler now a requirement to build the kernel? Or only to 
> run krobot tests?
> 
> Is this the earliest version of the compiler that can be used? I'm still 
> using 4.8 and 5.4, which seems to suffice for the kernel.
> 
> Googling about this has been fruitless.
> 
> > 
> > All error/warnings (new ones prefixed by >>):
> > 
> > In file included from include/linux/intel-iommu.h:32:0,
> >  from drivers/gpu/drm/i915/i915_drv.h:41,
> >  from drivers/gpu/drm/i915/i915_oa_bxt.c:31:
> > include/linux/iommu.h: In function 'iommu_debugfs_new_driver_dir':
> > > > include/linux/iommu.h:706:8: error: parameter name omitted
> > 
> >  struct dentry *iommu_debugfs_new_driver_dir(char *) {};
> > ^~
> > In file included from include/linux/intel-iommu.h:32:0,
> >  from drivers/gpu/drm/i915/i915_drv.h:41,
> >  from drivers/gpu/drm/i915/i915_oa_bxt.c:31:
> > > > include/linux/iommu.h:706:8: warning: control reaches end of non-void 
> > > > function [-Wreturn-type]
> > 
> >  struct dentry *iommu_debugfs_new_driver_dir(char *) {};
> > ^~
> > 
> > vim +706 include/linux/iommu.h
> > 
> > 700 
> > 701 #ifdef CONFIG_IOMMU_DEBUGFS
> > 702 void iommu_debugfs_setup(void);
> > 703 struct dentry *iommu_debugfs_new_driver_dir(char *);
> > 704 #else
> > 705 static inline void iommu_debugfs_setup(void) {}
> >   > 706 struct dentry *iommu_debugfs_new_driver_dir(char *) {};
> > 707 #endif
> > 708 
> 
> I have no problems with adding parameter names. But 
> scripts/checkpatch.pl doesn't seem to check for this, nor require it. 
> Should checkpatch be updated?

I'm pretty sure that's not feasible.

And when the compiler tells you you've stuffed up some
syntactical bit, why should checkpatch duplicate the
output error message too?

btw:  That's an unnecessary ; at the end of that non-void
function and it should probably be something like:

static inline struct dentry *iommu_debugfs_new_driver_dir(char *dir)
{
return NULL;
}

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] fix double ;;s in code

[Joe Perches]

On Tue, 2018-02-20 at 17:19 +1100, Michael Ellerman wrote:
> Daniel Vetter  writes:
> > On Sun, Feb 18, 2018 at 11:00:56AM +0100, Christophe LEROY wrote:
> > > Le 17/02/2018 à 22:19, Pavel Machek a écrit :
> > > > 
> > > > Fix double ;;'s in code.
> > > > 
> > > > Signed-off-by: Pavel Machek 
> > > 
> > > A summary of the files modified on top of the patch would help understand
> > > the impact.
> > > 
> > > A maybe there should be one patch by area, eg one for each arch specific
> > > modif and one for drivers/ and one for block/ ?
> > 
> > Yeah, pls split this into one patch per area, with a suitable patch
> > subject prefix. Look at git log of each file to get a feeling for what's
> > the standard in each area.
> 
> This part is actually pretty annoying.
> 
> I hacked up a script (below) which seems to do a reasonable job in most
> cases.

Here was another suggestion from awhile ago
https://lkml.org/lkml/2010/11/16/245

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] Intel-IOMMU: Delete an error message for a failed memory allocation in init_dmars()

[Joe Perches]

On Sun, 2018-01-21 at 08:19 +0100, Jörg Rödel wrote:
> On Sat, Jan 20, 2018 at 05:37:52PM -0800, Joe Perches wrote:
> > While Markus' commit messages are nearly universally terrible,
> > is there really any signficant value in knowing when any
> > particular OOM condition occurs other than the subsystem that
> > became OOM?
> > 
> > You're going to be hosed in any case.
> > 
> > Why isn't the generic OOM stack dump good enough?
> 
> Because if we know the exact allocation attempt that failed right away,
> we can more easily check if we can rewrite it so that it is more likely
> to succeed, e.g. rewriting one higher-order allocation to multiple
> order-0 allocations.

Up to you but I think that's pretty dubious
as this is an init function and if it fails
the system really is stuffed.

Unrelated, there are some unnecessary casts
of pointers to void * that could be removed
to help make the code a bit more regular as
some callers use the cast and some do not.

---
 drivers/iommu/intel-iommu.c | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 4a2de34895ec..8d7ea76345ae 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -845,8 +845,8 @@ static inline struct context_entry 
*iommu_context_addr(struct intel_iommu *iommu
if (!context)
return NULL;
 
-   __iommu_flush_cache(iommu, (void *)context, CONTEXT_SIZE);
-   phy_addr = virt_to_phys((void *)context);
+   __iommu_flush_cache(iommu, context, CONTEXT_SIZE);
+   phy_addr = virt_to_phys(context);
*entry = phy_addr | 1;
__iommu_flush_cache(iommu, entry, sizeof(*entry));
}
@@ -1298,7 +1298,7 @@ static int iommu_alloc_root_entry(struct intel_iommu 
*iommu)
struct root_entry *root;
unsigned long flags;
 
-   root = (struct root_entry *)alloc_pgtable_page(iommu->node);
+   root = alloc_pgtable_page(iommu->node);
if (!root) {
pr_err("Allocating root entry for %s failed\n",
iommu->name);
@@ -1978,7 +1978,7 @@ static int domain_init(struct dmar_domain *domain, struct 
intel_iommu *iommu,
domain->nid = iommu->node;
 
/* always allocate the top pgd */
-   domain->pgd = (struct dma_pte *)alloc_pgtable_page(domain->nid);
+   domain->pgd = alloc_pgtable_page(domain->nid);
if (!domain->pgd)
return -ENOMEM;
__iommu_flush_cache(iommu, domain->pgd, PAGE_SIZE);
@@ -4168,7 +4168,7 @@ int __init dmar_parse_one_rmrr(struct acpi_dmar_header 
*header, void *arg)
if (!rmrru->resv)
goto free_rmrru;
 
-   rmrru->devices = dmar_alloc_dev_scope((void *)(rmrr + 1),
+   rmrru->devices = dmar_alloc_dev_scope(rmrr + 1,
((void *)rmrr) + rmrr->header.length,
>devices_cnt);
if (rmrru->devices_cnt && rmrru->devices == NULL)
@@ -4229,7 +4229,7 @@ int dmar_parse_one_atsr(struct acpi_dmar_header *hdr, 
void *arg)
memcpy(atsru->hdr, hdr, hdr->length);
atsru->include_all = atsr->flags & 0x1;
if (!atsru->include_all) {
-   atsru->devices = dmar_alloc_dev_scope((void *)(atsr + 1),
+   atsru->devices = dmar_alloc_dev_scope(atsr + 1,
(void *)atsr + atsr->header.length,
>devices_cnt);
if (atsru->devices_cnt && atsru->devices == NULL) {
@@ -4465,7 +4465,7 @@ int dmar_iommu_notify_scope_dev(struct 
dmar_pci_notify_info *info)
rmrr = container_of(rmrru->hdr,
struct acpi_dmar_reserved_memory, header);
if (info->event == BUS_NOTIFY_ADD_DEVICE) {
-   ret = dmar_insert_dev_scope(info, (void *)(rmrr + 1),
+   ret = dmar_insert_dev_scope(info, rmrr + 1,
((void *)rmrr) + rmrr->header.length,
rmrr->segment, rmrru->devices,
rmrru->devices_cnt);
@@ -4483,7 +4483,7 @@ int dmar_iommu_notify_scope_dev(struct 
dmar_pci_notify_info *info)
 
atsr = container_of(atsru->hdr, struct acpi_dmar_atsr, header);
if (info->event == BUS_NOTIFY_ADD_DEVICE) {
-   ret = dmar_insert_dev_scope(info, (void *)(atsr + 1),
+   ret = dmar_insert_dev_scope(info, atsr + 1,
(void *)atsr + atsr->header.length,
atsr->segment, atsru->devices,
at

Re: [PATCH] Intel-IOMMU: Delete an error message for a failed memory allocation in init_dmars()

[Joe Perches]

On Sat, 2018-01-20 at 20:40 +0100, Jörg Rödel wrote:
> On Sat, Jan 20, 2018 at 03:55:37PM +0100, SF Markus Elfring wrote:
> > Do you need any more background information for this general
> > transformation pattern?
> 
> No.
> 
> > Do you find the Linux allocation failure report insufficient for this
> > use case?
> 
> Yes, because it can't tell me what the code was trying to allocate.

While Markus' commit messages are nearly universally terrible,
is there really any signficant value in knowing when any
particular OOM condition occurs other than the subsystem that
became OOM?

You're going to be hosed in any case.

Why isn't the generic OOM stack dump good enough?
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v10 1/9] perf/amd/iommu: Declare pr_fmt and remove unnecessary pr_debug

[Joe Perches]

On Wed, 2017-02-15 at 14:56 -0600, Suravee Suthikulpanit wrote:
> Declare pr_fmt for perf/amd_iommu and remove unnecessary pr_debug.
[]
> diff --git a/arch/x86/events/amd/iommu.c b/arch/x86/events/amd/iommu.c
[]
> @@ -11,6 +11,8 @@
>   * published by the Free Software Foundation.
>   */
>  
> +#define pr_fmt(fmt)  "perf/amd_iommu: " fmt
> +
>  #include 
>  #include 
>  #include 
[]
> @@ -444,24 +440,24 @@ static __init int _init_perf_amd_iommu(
>  
>   raw_spin_lock_init(_iommu->lock);
>  
> - /* Init format attributes */
>   perf_iommu->format_group = _iommu_format_group;
>  
>   /* Init cpumask attributes to only core 0 */
>   cpumask_set_cpu(0, _cpumask);
>   perf_iommu->cpumask_group = _iommu_cpumask_group;
>  
> - /* Init events attributes */
> - if (_init_events_attrs(perf_iommu) != 0)
> - pr_err("perf: amd_iommu: Only support raw events.\n");
> + ret = _init_events_attrs(perf_iommu);
> + if (ret) {
> + pr_err("Error initializing AMD IOMMU perf events.\n");
> + return ret;
> + }
>  
> - /* Init null attributes */
>   perf_iommu->null_group = NULL;
>   perf_iommu->pmu.attr_groups = perf_iommu->attr_groups;
>  
>   ret = perf_pmu_register(_iommu->pmu, name, -1);
>   if (ret) {
> - pr_err("perf: amd_iommu: Failed to initialized.\n");
> + pr_err("Error initializing AMD IOMMU perf counters.\n");
>   amd_iommu_pc_exit();
>   } else {
>   pr_info("perf: amd_iommu: Detected. (%d banks, %d 
> counters/bank)\n",

You should remove the now unnecessary internal prefixes
in the other pr_ uses like the pr_info above.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 1/2] iommu/vt-d: Ratelimit fault handler

[Joe Perches]

On Thu, 2016-03-17 at 14:12 -0600, Alex Williamson wrote:
> Fault rates can easily overwhelm the console and make the system
> unresponsive.  Ratelimit to allow an opportunity for maintenance.
[]
> diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
[]
> @@ -1602,10 +1602,17 @@ irqreturn_t dmar_fault(int irq, void *dev_id)
>   int reg, fault_index;
>   u32 fault_status;
>   unsigned long flag;
> + bool ratelimited;
> + static DEFINE_RATELIMIT_STATE(rs,
> +   DEFAULT_RATELIMIT_INTERVAL,
> +   DEFAULT_RATELIMIT_BURST);

Are these the appropriate limits for dmar?

include/linux/ratelimit.h:#define DEFAULT_RATELIMIT_INTERVAL(5 * HZ)
include/linux/ratelimit.h:#define DEFAULT_RATELIMIT_BURST   10


___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 0/2] iommu/vt-d: Fault logging improvements

[Joe Perches]

On Thu, 2016-03-17 at 14:12 -0600, Alex Williamson wrote:
> Ratelimit and improve formatting.

Makes sense, thanks.

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] iommu/vt-d: Ratelimit fault handler

[Joe Perches]

On Tue, 2016-03-15 at 10:35 -0600, Alex Williamson wrote:
> Fault rates can easily overwhelm the console and make the system
> unresponsive.  Ratelimit to allow an opportunity for maintenance.

A few suggestions:

o Use a single ratelimit state.
o The multiple lines output are unnecessary and hard to grep
  in the dmesg output because of inconsistent prefixing as
  second and subsequent output lines are not prefixed by pr_fmt.
o The DMAR prefix on the second block is also unnecessary as
  it's already prefixed by pr_fmt
o Coalesce the formats for easier grep.

so maybe:
---
 drivers/iommu/dmar.c | 28 
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
index 8ffd756..59dcaaa 100644
--- a/drivers/iommu/dmar.c
+++ b/drivers/iommu/dmar.c
@@ -1575,23 +1575,27 @@ static int dmar_fault_do_one(struct intel_iommu *iommu, 
int type,
 {
    const char *reason;
    int fault_type;
+   static DEFINE_RATELIMIT_STATE(rs,
+     DEFAULT_RATELIMIT_INTERVAL,
+     DEFAULT_RATELIMIT_BURST);
+
+   if (__ratelimit())
+   return 0;
 
    reason = dmar_get_fault_reason(fault_reason, _type);
 
    if (fault_type == INTR_REMAP)
-   pr_err("INTR-REMAP: Request device [[%02x:%02x.%d] "
-      "fault index %llx\n"
-   "INTR-REMAP:[fault reason %02d] %s\n",
-   (source_id >> 8), PCI_SLOT(source_id & 0xFF),
-   PCI_FUNC(source_id & 0xFF), addr >> 48,
-   fault_reason, reason);
+   pr_err("[INTR-REMAP] Request device [%02x:%02x.%d] fault index 
%llx [fault reason %02d] %s\n",
+      source_id >> 8, PCI_SLOT(source_id & 0xFF),
+      PCI_FUNC(source_id & 0xFF), addr >> 48,
+      fault_reason, reason);
    else
-   pr_err("DMAR:[%s] Request device [%02x:%02x.%d] "
-      "fault addr %llx \n"
-      "DMAR:[fault reason %02d] %s\n",
-      (type ? "DMA Read" : "DMA Write"),
-      (source_id >> 8), PCI_SLOT(source_id & 0xFF),
-      PCI_FUNC(source_id & 0xFF), addr, fault_reason, reason);
+   pr_err("[%s] Request device [%02x:%02x.%d] fault addr %llx 
[fault reason %02d] %s\n",
+      type ? "DMA Read" : "DMA Write",
+      source_id >> 8, PCI_SLOT(source_id & 0xFF),
+      PCI_FUNC(source_id & 0xFF), addr,
+      fault_reason, reason);
+
    return 0;
 }
 

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] iommu/vt-d: Ratelimit fault handler

[Joe Perches]

On Tue, 2016-03-15 at 10:10 -0700, Joe Perches wrote:
> o Use a single ratelimit state.
[]
> diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
[]
> + if (__ratelimit())
> + return 0;

That of course should be:

if (!__ratelimit())
return 0;
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] iommu: Update my email address

[Joe Perches]

On Wed, 2015-02-04 at 16:17 +0100, Joerg Roedel wrote:
 The AMD address is dead for a long time already, replace it
 with a working one.

Maybe add a .mailmap entry too.


___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 06/28] iommu/amd: Move informational prinks out of iommu_enable

[Joe Perches]

On Thu, 2012-07-05 at 14:36 +0200, Joerg Roedel wrote:
 This function will be called before the PCI subsystem is
 initialized. Therefore dev_name doen't work and IOMMU
 information can't be printed to the klog as before. Move the
 code to print that information to a later point where PCI
 initializtion has already happened.
[]
 diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
[]
 @@ -1102,6 +1085,28 @@ static int iommu_init_pci(struct amd_iommu *iommu)
[]
 +static void print_iommu_info(void)
 +{
 + static const char * const feat_str[] = {
 + PreF, PPR, X2APIC, NX, GT, [5],
 + IA, GA, HE, PC, NULL
 + };
 + struct amd_iommu *iommu;
 +
 + for_each_iommu(iommu) {
 + int i;
 + pr_info(AMD-Vi: Found IOMMU at %s cap 0x%hx\n,
 + dev_name(iommu-dev-dev), iommu-cap_ptr);
 + if (iommu-cap  (1  IOMMU_CAP_EFR)) {
 + pr_info(AMD-Vi:  Extended features: );
 + for (i = 0; feat_str[i]; ++i)
 + if (iommu_feature(iommu, (1ULL  i)))
 + pr_cont( %s, feat_str[i]);

I think this should use {} around the for loop
and this would be better as:

static const char * const feat_str[] = {
PreF, PPR, X2APIC, NX, GT, [5],
IA, GA, HE, PC
};
[]
for (i = 0; ARRAY_SIZE(feat_str); i++) {
if (iommu_feature(iommu, (1ULL  i)))
pr_cont( %s, feat_str[i]);
}

I don't see the utility of the separate function and
this could just be inlined in the calling function.

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 1/2] iommu: dmar: replace printks with appropriate pr_*()

[Joe Perches]

On Mon, 2012-06-04 at 17:29 -0400, Donald Dutile wrote:
 Replace printk(KERN_*  with pr_*() functions.

Please add
#define pr_fmt(fmt) KBUILD_MODNAME :  fmt
before any include and remove the embedded PREFIX
from each printk

 diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c

[]

   break;
   }
   pdev = pci_get_slot(bus, PCI_DEVFN(path-dev, path-fn));
   if (!pdev) {
 - printk(KERN_WARNING PREFIX
 - Device scope device [%04x:%02x:%02x.%02x] not found\n,
 + pr_warn(PREFIX Device scope device
 + [%04x:%02x:%02x.%02x] not found\n,
   segment, bus-number, path-dev, path-fn);

Please don't split any format string.  You removed
a space between the scope device and an open bracket.
It's OK for format strings to exceed 80 chars.


___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu