Re: [PATCH v13 25/35] KVM: selftests: Convert lib's mem regions to KVM_SET_USER_MEMORY_REGION2
On Fri, Oct 27, 2023 at 11:22:07AM -0700, Sean Christopherson wrote: > Use KVM_SET_USER_MEMORY_REGION2 throughout KVM's selftests library so that > support for guest private memory can be added without needing an entirely > separate set of helpers. > > Note, this obviously makes selftests backwards-incompatible with older KVM ^^ > versions from this point forward. ^ Is there a way we could disable the tests on older kernels instead of making them fail? Check uname or something? There is probably a standard way to do this... It's these tests which fail. kvm_aarch32_id_regs kvm_access_tracking_perf_test kvm_arch_timer kvm_debug-exceptions kvm_demand_paging_test kvm_dirty_log_perf_test kvm_dirty_log_test kvm_guest_print_test kvm_hypercalls kvm_kvm_page_table_test kvm_memslot_modification_stress_test kvm_memslot_perf_test kvm_page_fault_test kvm_psci_test kvm_rseq_test kvm_smccc_filter kvm_steal_time kvm_vgic_init kvm_vgic_irq kvm_vpmu_counter_access regards, dan carpenter
Re: [PATCH] ibmvnic: Use -EBUSY in __ibmvnic_reset()
On Tue, Apr 23, 2024 at 09:55:57AM -0500, Nick Child wrote:
> > You're right that it doesn't affect the behavior of the driver except
> > for the debug output when we do:
> >
> > netdev_dbg(adapter->netdev, "Reset failed, rc=%d\n", rc);
> >
> > But the - was left off uninitentionally so I think we should apply it.
> >
> > I have been trying to look for similar bugs where the - is left off.
> > It's a bit challenging because there places where we use positive
> > error codes deliberately. But in this case a static checker could
> > easily detect the bug with a low false positive ratio by saying, "We're
> > mixing normal negative error codes with positive EBUSY".
> >
> > regards,
> > dan carpenter
>
> Hello, small clarification, this patch will not effect the debug print
> statement due to the break statement immediately following:
> while () {
> if () {
> rc = -EBUSY;
> break;
> }
> netdev_dbg(adapter->netdev, "Reset failed, rc=%d\n", rc);
> }
> Though this return code can be passed to adapter->reset_done_rc, which is
> only treated as a boolean.
>
> So, the point of the patch not doing any behavioral differences is still
> true.
Ah yes. You're right.
regards,
dan carpenter
Re: [PATCH] ibmvnic: Use -EBUSY in __ibmvnic_reset()
On Tue, Apr 23, 2024 at 12:54:55PM +0200, Paolo Abeni wrote:
> On Fri, 2024-04-19 at 16:08 +0200, Markus Elfring wrote:
> > From: Markus Elfring
> > Date: Fri, 19 Apr 2024 15:46:17 +0200
> >
> > Add a minus sign before the error code “EBUSY”
> > so that a negative value will be used as in other cases.
> >
> > This issue was transformed by using the Coccinelle software.
> >
> > Signed-off-by: Markus Elfring
> > ---
> > drivers/net/ethernet/ibm/ibmvnic.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/ibm/ibmvnic.c
> > b/drivers/net/ethernet/ibm/ibmvnic.c
> > index 5e9a93bdb518..737ae83a836a 100644
> > --- a/drivers/net/ethernet/ibm/ibmvnic.c
> > +++ b/drivers/net/ethernet/ibm/ibmvnic.c
> > @@ -3212,7 +3212,7 @@ static void __ibmvnic_reset(struct work_struct *work)
> > adapter->state == VNIC_REMOVED) {
> > spin_unlock_irqrestore(>state_lock, flags);
> > kfree(rwi);
> > - rc = EBUSY;
> > + rc = -EBUSY;
> > break;
> >
>
> AFAICS the error is always used as bool, so this will not change any
> behavior in practice. I tend to think we should not merge this kind of
> change outside some larger work in the same area, but I'd love a second
> opinion from the driver owners.
I missed the original patch due to my procmail filters...
You're right that it doesn't affect the behavior of the driver except
for the debug output when we do:
netdev_dbg(adapter->netdev, "Reset failed, rc=%d\n", rc);
But the - was left off uninitentionally so I think we should apply it.
I have been trying to look for similar bugs where the - is left off.
It's a bit challenging because there places where we use positive
error codes deliberately. But in this case a static checker could
easily detect the bug with a low false positive ratio by saying, "We're
mixing normal negative error codes with positive EBUSY".
regards,
dan carpenter
Re: powerpc: io-defs.h:43:1: error: performing pointer arithmetic on a null pointer has undefined behavior [-Werror,-Wnull-pointer-arithmetic]
On Tue, Apr 16, 2024 at 01:55:57PM +0200, Arnd Bergmann wrote: > On Tue, Apr 16, 2024, at 13:01, Arnd Bergmann wrote: > > On Tue, Apr 16, 2024, at 11:32, Naresh Kamboju wrote: > >> The Powerpc clang builds failed due to following warnings / errors on the > >> Linux next-20240416 tag. > >> > >> Powerpc: > >> - tqm8xx_defconfig + clang-17 - Failed > >> - allnoconfig + clang-17 - Failed > >> - tinyconfig + clang-17 - Failed > >> > >> Reported-by: Linux Kernel Functional Testing > > > > I'm not sure why this showed up now, but there is a series from > > in progress that will avoid this in the future, as the same > > issue is present on a couple of other architectures. > > > > I see now, it was introduced by my patch to turn on -Wextra > by default. I had tested that patch on all architectures > with allmodconfig and defconfig, but I did not test any > powerpc configs with PCI disabled. I think this warning is clang specific as well... (Maybe clang was included in all architectures but I'm not sure). regards, dan carpenter
Re: [PATCH 00/14] Add support for suppressing warning backtraces
Thanks! Acked-by: Dan Carpenter regards, dan carpenter
Re: [PATCH RFC net] ps3/gelic: Fix possible NULL pointer dereference
This driver is PPC so I have never looked at the code before. I noticed
another issue that was introduced last December in commit 3ce4f9c3fbb3
("net/ps3_gelic_net: Add gelic_descr structures").
net/ethernet/toshiba/ps3_gelic_net.c
375 static int gelic_descr_prepare_rx(struct gelic_card *card,
376struct gelic_descr *descr)
377 {
378 static const unsigned int rx_skb_size =
379 ALIGN(GELIC_NET_MAX_FRAME, GELIC_NET_RXBUF_ALIGN) +
380 GELIC_NET_RXBUF_ALIGN - 1;
381 dma_addr_t cpu_addr;
382 int offset;
383
384 if (gelic_descr_get_status(descr) !=
GELIC_DESCR_DMA_NOT_IN_USE)
385 dev_info(ctodev(card), "%s: ERROR status\n", __func__);
386
387 descr->skb = netdev_alloc_skb(*card->netdev, rx_skb_size);
388 if (!descr->skb) {
389 descr->hw_regs.payload.dev_addr = 0; /* tell DMAC don't
touch memory */
390 return -ENOMEM;
391 }
392 descr->hw_regs.dmac_cmd_status = 0;
393 descr->hw_regs.result_size = 0;
394 descr->hw_regs.valid_size = 0;
395 descr->hw_regs.data_error = 0;
396 descr->hw_regs.payload.dev_addr = 0;
397 descr->hw_regs.payload.size = 0;
398 descr->skb = NULL;
^^
NULL
399
400 offset = ((unsigned long)descr->skb->data) &
Dereferenced here.
401 (GELIC_NET_RXBUF_ALIGN - 1);
402 if (offset)
403 skb_reserve(descr->skb, GELIC_NET_RXBUF_ALIGN - offset);
404 /* io-mmu-map the skb */
405 cpu_addr = dma_map_single(ctodev(card), descr->skb->data,
406 GELIC_NET_MAX_FRAME, DMA_FROM_DEVICE);
regards,
dan carpenter
Re: [PATCH v3 00/18] fbdev: Remove FBINFO_DEFAULT and FBINFO_FLAG_DEFAULT flags
On Fri, Jul 14, 2023 at 12:24:05PM +0200, Thomas Zimmermann wrote: > > > > >fbdev: Remove flag FBINFO_DEFAULT from fbdev drivers > > >fbdev: Remove flag FBINFO_DEFAULT from fbdev drivers > > >fbdev: Remove flag FBINFO_DEFAULT from fbdev drivers > > >fbdev: Remove flag FBINFO_DEFAULT from fbdev drivers > > > I wasn't happy about this either. But I could not come up with a description > that fits into the 74-char limit for each commit. They only differ in the > method of memory allocation. Do you have any ideas? fbdev: Remove FBINFO_DEFAULT from static structs fbdev: Remove FBINFO_DEFAULT from kzalloc() structs fbdev: Remove FBINFO_DEFAULT from devm_kzalloc() structs regards, dan carpenter
Re: [PATCH net-next v2 01/10] net: wan: Remove unnecessary (void*) conversions
On Mon, Jul 10, 2023 at 02:39:33PM +0800, Su Hui wrote:
> From: wuych
^
This doesn't look like a real name.
>
> Pointer variables of void * type do not require type cast.
>
> Signed-off-by: wuych
> ---
> drivers/net/wan/fsl_ucc_hdlc.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/wan/fsl_ucc_hdlc.c b/drivers/net/wan/fsl_ucc_hdlc.c
> index 47c2ad7a3e42..73c73d8f4bb2 100644
> --- a/drivers/net/wan/fsl_ucc_hdlc.c
> +++ b/drivers/net/wan/fsl_ucc_hdlc.c
> @@ -350,11 +350,11 @@ static int uhdlc_init(struct ucc_hdlc_private *priv)
> static netdev_tx_t ucc_hdlc_tx(struct sk_buff *skb, struct net_device *dev)
> {
> hdlc_device *hdlc = dev_to_hdlc(dev);
> - struct ucc_hdlc_private *priv = (struct ucc_hdlc_private *)hdlc->priv;
> - struct qe_bd *bd;
> - u16 bd_status;
> + struct ucc_hdlc_private *priv = hdlc->priv;
> unsigned long flags;
> __be16 *proto_head;
> + struct qe_bd *bd;
> + u16 bd_status;
Don't move the other variables around. That's unrelated to the cast.
(Same applies to all the other patches).
regards,
dan carpenter
Re: [PATCH 1/2] ASoC: fsl_mqs: move of_node_put() to the correct location
On Mon, Apr 03, 2023 at 11:26:47PM +0800, Liliang Ye wrote:
> of_node_put() should have been done directly after
> mqs_priv->regmap = syscon_node_to_regmap(gpr_np);
> otherwise it creates a reference leak on the success path.
>
> To fix this, of_node_put() is moved to the correct location, and change
> all the gotos to direct returns.
>
> Fixes: a9d273671440 ("ASoC: fsl_mqs: Fix error handling in probe")
> Signed-off-by: Liliang Ye
> Reviewed-by: Dan Carpenter
> ---
These patches are from a university. I think this patch was based on
manual review rather than static analysis. They have not been tested
and this one affects the success path so that's always extra risky.
However I do believe the patch is correct and reviewed it before it was
sent publically.
regards,
dan carpenter
Re: [PATCH v4 1/4] PCI: Introduce pci_dev_for_each_resource()
Hi Andy,
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url:
https://github.com/intel-lab-lkp/linux/commits/Andy-Shevchenko/PCI-Introduce-pci_dev_for_each_resource/20230311-011642
base: https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
patch link:
https://lore.kernel.org/r/20230310171416.23356-2-andriy.shevchenko%40linux.intel.com
patch subject: [PATCH v4 1/4] PCI: Introduce pci_dev_for_each_resource()
config: x86_64-randconfig-m001
(https://download.01.org/0day-ci/archive/20230311/202303112149.xd47qkoy-...@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot
| Reported-by: Dan Carpenter
| Link: https://lore.kernel.org/r/202303112149.xd47qkoy-...@intel.com/
smatch warnings:
drivers/pnp/quirks.c:248 quirk_system_pci_resources() warn: was && intended
here instead of ||?
vim +248 drivers/pnp/quirks.c
0509ad5e1a7d92 Bjorn Helgaas 2008-03-11 229 static void
quirk_system_pci_resources(struct pnp_dev *dev)
0509ad5e1a7d92 Bjorn Helgaas 2008-03-11 230 {
0509ad5e1a7d92 Bjorn Helgaas 2008-03-11 231 struct pci_dev *pdev =
NULL;
059b4a086017fb Mika Westerberg 2023-03-10 232 struct resource *res,
*r;
0509ad5e1a7d92 Bjorn Helgaas 2008-03-11 233 int i, j;
0509ad5e1a7d92 Bjorn Helgaas 2008-03-11 234
0509ad5e1a7d92 Bjorn Helgaas 2008-03-11 235 /*
0509ad5e1a7d92 Bjorn Helgaas 2008-03-11 236 * Some BIOSes have PNP
motherboard devices with resources that
0509ad5e1a7d92 Bjorn Helgaas 2008-03-11 237 * partially overlap
PCI BARs. The PNP system driver claims these
0509ad5e1a7d92 Bjorn Helgaas 2008-03-11 238 * motherboard
resources, which prevents the normal PCI driver from
0509ad5e1a7d92 Bjorn Helgaas 2008-03-11 239 * requesting them
later.
0509ad5e1a7d92 Bjorn Helgaas 2008-03-11 240 *
0509ad5e1a7d92 Bjorn Helgaas 2008-03-11 241 * This patch disables
the PNP resources that conflict with PCI BARs
0509ad5e1a7d92 Bjorn Helgaas 2008-03-11 242 * so they won't be
claimed by the PNP system driver.
0509ad5e1a7d92 Bjorn Helgaas 2008-03-11 243 */
0509ad5e1a7d92 Bjorn Helgaas 2008-03-11 244 for_each_pci_dev(pdev) {
059b4a086017fb Mika Westerberg 2023-03-10 245
pci_dev_for_each_resource(pdev, r, i) {
059b4a086017fb Mika Westerberg 2023-03-10 246
unsigned long type = resource_type(r);
999ed65ad12e37 Rene Herman 2008-07-25 247
059b4a086017fb Mika Westerberg 2023-03-10 @248 if
(type != IORESOURCE_IO || type != IORESOURCE_MEM ||
^^
This || needs to be &&. This loop will always hit the continue path
without doing anything.
059b4a086017fb Mika Westerberg 2023-03-10 249
resource_size(r) == 0)
0509ad5e1a7d92 Bjorn Helgaas 2008-03-11 250
continue;
0509ad5e1a7d92 Bjorn Helgaas 2008-03-11 251
059b4a086017fb Mika Westerberg 2023-03-10 252 if
(r->flags & IORESOURCE_UNSET)
f7834c092c4299 Bjorn Helgaas 2015-03-03 253
continue;
f7834c092c4299 Bjorn Helgaas 2015-03-03 254
95ab3669f78306 Bjorn Helgaas 2008-04-28 255 for (j
= 0;
999ed65ad12e37 Rene Herman 2008-07-25 256
(res = pnp_get_resource(dev, type, j)); j++) {
aee3ad815dd291 Bjorn Helgaas 2008-06-27 257
if (res->start == 0 && res->end == 0)
0509ad5e1a7d92 Bjorn Helgaas 2008-03-11 258
continue;
0509ad5e1a7d92 Bjorn Helgaas 2008-03-11 259
0509ad5e1a7d92 Bjorn Helgaas 2008-03-11 260
/*
0509ad5e1a7d92 Bjorn Helgaas 2008-03-11 261
* If the PNP region doesn't overlap the PCI
0509ad5e1a7d92 Bjorn Helgaas 2008-03-11 262
* region at all, there's no problem.
0509ad5e1a7d92 Bjorn Helgaas 2008-03-11 263
*/
059b4a086017fb Mika Westerberg 2023-03-10 264
if (!resource_overlaps(res, r))
0509ad5e1a7d92 Bjorn Helgaas 2008-03-11 265
continue;
0509ad5e1a7d92 Bjorn Helgaas 2008-03-11 266
0509ad5e1a7d92 Bjorn Helgaas 2008-03-11 267
/*
0509ad5e1a7d92 Bjorn Helgaas 2008-03-11 268
* If the PNP region completely encloses (or is
0509ad5e1a7d92 Bjorn Helgaas 2008-03-11 269
* at least as large as) the PCI region, that's
0509ad5e1
Re: [PATCH] i2c: pasemi: Add IRQ support for Apple Silicon
Hi Arminder,
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url:
https://github.com/intel-lab-lkp/linux/commits/Arminder-Singh/i2c-pasemi-Add-IRQ-support-for-Apple-Silicon/20220821-034703
base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: parisc-randconfig-m031-20220821
(https://download.01.org/0day-ci/archive/20220822/202208220231.f88sizqa-...@intel.com/config)
compiler: hppa-linux-gcc (GCC) 12.1.0
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot
Reported-by: Dan Carpenter
smatch warnings:
drivers/i2c/busses/i2c-pasemi-core.c:92 pasemi_smb_waitready() error:
uninitialized symbol 'status'.
vim +/status +92 drivers/i2c/busses/i2c-pasemi-core.c
8032214346c0c8 drivers/i2c/busses/i2c-pasemi.c Julia Lawall 2010-09-05
80 static int pasemi_smb_waitready(struct pasemi_smbus *smbus)
beb58aa39e6e5a drivers/i2c/busses/i2c-pasemi.c Olof Johansson 2007-02-13
81 {
beb58aa39e6e5a drivers/i2c/busses/i2c-pasemi.c Olof Johansson 2007-02-13
82 int timeout = 10;
beb58aa39e6e5a drivers/i2c/busses/i2c-pasemi.c Olof Johansson 2007-02-13
83 unsigned int status;
98584b2b51d808 drivers/i2c/busses/i2c-pasemi-core.c Arminder Singh 2022-08-20
84 unsigned int bitmask = SMSTA_XEN | SMSTA_MTN;
beb58aa39e6e5a drivers/i2c/busses/i2c-pasemi.c Olof Johansson 2007-02-13
85
98584b2b51d808 drivers/i2c/busses/i2c-pasemi-core.c Arminder Singh 2022-08-20
86 if (smbus->use_irq) {
98584b2b51d808 drivers/i2c/busses/i2c-pasemi-core.c Arminder Singh 2022-08-20
87 reinit_completion(>irq_completion);
98584b2b51d808 drivers/i2c/busses/i2c-pasemi-core.c Arminder Singh 2022-08-20
88 reg_write(smbus, REG_IMASK, bitmask);
98584b2b51d808 drivers/i2c/busses/i2c-pasemi-core.c Arminder Singh 2022-08-20
89 wait_for_completion_timeout(>irq_completion,
msecs_to_jiffies(10));
beb58aa39e6e5a drivers/i2c/busses/i2c-pasemi.c Olof Johansson 2007-02-13
90 status = reg_read(smbus, REG_SMSTA);
98584b2b51d808 drivers/i2c/busses/i2c-pasemi-core.c Arminder Singh 2022-08-20
91 } else {
beb58aa39e6e5a drivers/i2c/busses/i2c-pasemi.c Olof Johansson 2007-02-13
@92 while (!(status & SMSTA_XEN) && timeout--) {
"status" not intialized.
beb58aa39e6e5a drivers/i2c/busses/i2c-pasemi.c Olof Johansson 2007-02-13
93 msleep(1);
beb58aa39e6e5a drivers/i2c/busses/i2c-pasemi.c Olof Johansson 2007-02-13
94 status = reg_read(smbus, REG_SMSTA);
beb58aa39e6e5a drivers/i2c/busses/i2c-pasemi.c Olof Johansson 2007-02-13
95 }
98584b2b51d808 drivers/i2c/busses/i2c-pasemi-core.c Arminder Singh 2022-08-20
96 }
98584b2b51d808 drivers/i2c/busses/i2c-pasemi-core.c Arminder Singh 2022-08-20
97
beb58aa39e6e5a drivers/i2c/busses/i2c-pasemi.c Olof Johansson 2007-02-13
98
be8a1f7cd4501c drivers/i2c/busses/i2c-pasemi.c Olof Johansson 2007-11-15
99 /* Got NACK? */
be8a1f7cd4501c drivers/i2c/busses/i2c-pasemi.c Olof Johansson 2007-11-15
100 if (status & SMSTA_MTN)
be8a1f7cd4501c drivers/i2c/busses/i2c-pasemi.c Olof Johansson 2007-11-15
101 return -ENXIO;
be8a1f7cd4501c drivers/i2c/busses/i2c-pasemi.c Olof Johansson 2007-11-15
102
beb58aa39e6e5a drivers/i2c/busses/i2c-pasemi.c Olof Johansson 2007-02-13
103 if (timeout < 0) {
c06f50ed36cc0a drivers/i2c/busses/i2c-pasemi.c Sven Peter 2021-10-08
104 dev_warn(smbus->dev, "Timeout, status 0x%08x\n", status);
beb58aa39e6e5a drivers/i2c/busses/i2c-pasemi.c Olof Johansson 2007-02-13
105 reg_write(smbus, REG_SMSTA, status);
beb58aa39e6e5a drivers/i2c/busses/i2c-pasemi.c Olof Johansson 2007-02-13
106 return -ETIME;
beb58aa39e6e5a drivers/i2c/busses/i2c-pasemi.c Olof Johansson 2007-02-13
107 }
beb58aa39e6e5a drivers/i2c/busses/i2c-pasemi.c Olof Johansson 2007-02-13
108
beb58aa39e6e5a drivers/i2c/busses/i2c-pasemi.c Olof Johansson 2007-02-13
109 /* Clear XEN */
beb58aa39e6e5a drivers/i2c/busses/i2c-pasemi.c Olof Johansson 2007-02-13
110 reg_write(smbus, REG_SMSTA, SMSTA_XEN);
beb58aa39e6e5a drivers/i2c/busses/i2c-pasemi.c Olof Johansson 2007-02-13
111
beb58aa39e6e5a drivers/i2c/busses/i2c-pasemi.c Olof Johansson 2007-02-13
112 return 0;
beb58aa39e6e5a drivers/i2c/busses/i2c-pasemi.c Olof Johansson 2007-02-13
113 }
--
0-DAY CI Kernel Test Service
https://01.org/lkp
Re: [linux-next:master] BUILD REGRESSION 088b9c375534d905a4d337c78db3b3bfbb52c4a0
@vger.kernel.org, io...@lists.linux-foundation.org, keyri...@vger.kernel.org, patc...@opensource.cirrus.com, k...@vger.kernel.org, da...@lists.linux.dev, linux...@kvack.org, accessrunner-gene...@lists.sourceforge.net, linux1394-de...@lists.sourceforge.net, linux-l...@vger.kernel.org, rds-de...@oss.oracle.com, linux-...@vger.kernel.org, d...@vger.kernel.org, intel-wired-...@lists.osuosl.org, linux-ser...@vger.kernel.org, devicet...@vger.kernel.org, linux-...@lists.01.org, osmocom-net-g...@lists.osmocom.org, appar...@lists.ubuntu.com, linux-r...@vger.kernel.org, linux-bca...@vger.kernel.org, linux-media...@lists.infradead.org, linux-te...@vger.kernel.org, linux-s...@vger.kernel.org, net...@vger.kernel.org, linux-unio...@vger.kernel.org, linux-blueto...@vger.kernel.org, n...@lists.linux.dev, tipc-discuss...@lists.sourceforge.net, linuxppc-dev@lists.ozlabs.org, linux-bt...@vger.kernel.org Errors-To: linuxppc-dev-bounces+archive=mail-archive@lists.ozlabs.org Sender: "Linuxppc-dev" On Thu, Jul 07, 2022 at 07:02:58AM -0700, Guenter Roeck wrote: > and the NULL > dereferences in the binder driver are at the very least suspicious. The NULL dereferences in binder are just nonsense Sparse annotations. They don't affect runtime. drivers/android/binder.c:1481:19-23: ERROR: from is NULL but dereferenced. drivers/android/binder.c:2920:29-33: ERROR: target_thread is NULL but dereferenced. drivers/android/binder.c:353:25-35: ERROR: node -> proc is NULL but dereferenced. drivers/android/binder.c:4888:16-20: ERROR: t is NULL but dereferenced. regards, dan carpenter
[PATCH] soc: fsl: guts: fix IS_ERR() vs NULL bug
The of_iomap() function returns NULL on failure, it never returns
error pointers.
Fixes: ab4988d6a393 ("soc: fsl: guts: embed fsl_guts_get_svr() in probe()")
Signed-off-by: Dan Carpenter
---
drivers/soc/fsl/guts.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/soc/fsl/guts.c b/drivers/soc/fsl/guts.c
index 27035de062f8..8038c599ad83 100644
--- a/drivers/soc/fsl/guts.c
+++ b/drivers/soc/fsl/guts.c
@@ -195,9 +195,9 @@ static int __init fsl_guts_init(void)
soc_data = match->data;
regs = of_iomap(np, 0);
- if (IS_ERR(regs)) {
+ if (!regs) {
of_node_put(np);
- return PTR_ERR(regs);
+ return -ENOMEM;
}
little_endian = of_property_read_bool(np, "little-endian");
--
2.35.1
Re: [PATCH 0/6] Remove usage of list iterator past the loop body
Updating this API is risky because some places rely on the old behavior and not all of them have been updated. Here are some additional places you might want to change. drivers/usb/host/uhci-q.c:466 link_async() warn: iterator used outside loop: 'pqh' drivers/infiniband/core/mad.c:968 ib_get_rmpp_segment() warn: iterator used outside loop: 'mad_send_wr->cur_seg' drivers/opp/debugfs.c:208 opp_migrate_dentry() warn: iterator used outside loop: 'new_dev' drivers/staging/greybus/audio_codec.c:602 gbcodec_mute_stream() warn: iterator used outside loop: 'module' drivers/staging/media/atomisp/pci/atomisp_acc.c:508 atomisp_acc_load_extensions() warn: iterator used outside loop: 'acc_fw' drivers/perf/thunderx2_pmu.c:814 tx2_uncore_pmu_init_dev() warn: iterator used outside loop: 'rentry' drivers/gpu/drm/nouveau/nvkm/engine/device/ctrl.c:111 nvkm_control_mthd_pstate_attr() warn: iterator used outside loop: 'pstate' drivers/gpu/drm/panfrost/panfrost_mmu.c:203 panfrost_mmu_as_get() warn: iterator used outside loop: 'lru_mmu' drivers/media/usb/uvc/uvc_v4l2.c:885 uvc_ioctl_enum_input() warn: iterator used outside loop: 'iterm' drivers/media/usb/uvc/uvc_v4l2.c:896 uvc_ioctl_enum_input() warn: iterator used outside loop: 'iterm' drivers/scsi/dc395x.c:3596 device_alloc() warn: iterator used outside loop: 'p' drivers/net/ethernet/mellanox/mlx4/alloc.c:379 __mlx4_alloc_from_zone() warn: iterator used outside loop: 'curr_node' fs/ocfs2/dlm/dlmdebug.c:573 lockres_seq_start() warn: iterator used outside loop: 'res' This patchset fixes 3 bugs. Initially when it's merged it's probably going to introduce some bugs because there are likely other places which rely on the old behavior. In an ideal world, with the new API the compiler would warn about uninitialized variables, but unfortunately that warning is disabled by default so we still have to rely on kbuild/Clang/Smatch to find the bugs. But hopefully the new API encourages people to write clearer code so it prevents bugs in the long run. regards, dan carpenter
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Wed, Mar 02, 2022 at 10:29:31AM +0100, Rasmus Villemoes wrote:
> This won't help the current issue (because it doesn't exist and might
> never), but just in case some compiler people are listening, I'd like to
> have some sort of way to tell the compiler "treat this variable as
> uninitialized from here on". So one could do
>
> #define kfree(p) do { __kfree(p); __magic_uninit(p); } while (0)
>
I think this is a good idea.
Smatch can already find all the iterator used outside the loop bugs that
Jakob did with a manageably small number of false positives. The
problems are that:
1) It would be better to find it in the compile stage instead of later.
2) I hadn't published that check. Will do shortly.
3) A couple weeks back I noticed that the list_for_each_entry() check
was no longer working. Fixed now.
4) Smatch was only looking at cases which dereferenced the iterator and
not checks for NULL. I will test the fix for that tonight.
5) Smatch is broken on PowerPC.
Coccinelle also has checks for iterator used outside the loop.
Coccinelle had these checks before Smatch did. I copied Julia's idea.
If your annotation was added to GCC it would solve all those problems.
But it's kind of awkward that we can't annotate kfree() directly
instead of creating the kfree() macro. And there are lots of other
functions which free things so you'd have to create a ton of macros
like:
#define gr_free_dma_desc(a, b) do { __gr_free_dma_desc(a, b);
__magic_uninit(b); } while (0)
And then there are functions which free a struct member:
void free_bar(struct foo *p) { kfree(p->bar); }
Or functions which free a container_of().
Smatch is more evolved than designed but what I do these days is use $0,
$1, $2 to represent the parameters. So you can say a function frees
$0->bar. For container_of() then is "(168<~$0)->bar" which means 168
bytes from $0. Returns are parameter -1 so I guess it would be $(-1),
but as I said Smatch evolved so right now places that talk about
returned values use a different format.
What you could do is just make a parseable table next to the function
definition with all the information. Then you would use a Perl script
to automatically generate a Coccinelle check to warn about use after
frees.
diff --git a/mm/slab.c b/mm/slab.c
index ddf5737c63d9..c9dffa5c40a2 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -3771,6 +3771,9 @@ EXPORT_SYMBOL(kmem_cache_free_bulk);
*
* Don't free memory not originally allocated by kmalloc()
* or you will run into trouble.
+ *
+ * CHECKER information
+ * frees: $0
*/
void kfree(const void *objp)
{
regards,
dan carpenter
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Wed, Mar 02, 2022 at 12:07:04PM -0800, Kees Cook wrote:
> On Wed, Mar 02, 2022 at 10:29:31AM +0100, Rasmus Villemoes wrote:
> > This won't help the current issue (because it doesn't exist and might
> > never), but just in case some compiler people are listening, I'd like to
> > have some sort of way to tell the compiler "treat this variable as
> > uninitialized from here on". So one could do
> >
> > #define kfree(p) do { __kfree(p); __magic_uninit(p); } while (0)
> >
> > with __magic_uninit being a magic no-op that doesn't affect the
> > semantics of the code, but could be used by the compiler's "[is/may be]
> > used uninitialized" machinery to flag e.g. double frees on some odd
> > error path etc. It would probably only work for local automatic
> > variables, but it should be possible to just ignore the hint if p is
> > some expression like foo->bar or has side effects. If we had that, the
> > end-of-loop test could include that to "uninitialize" the iterator.
>
> I've long wanted to change kfree() to explicitly set pointers to NULL on
> free. https://github.com/KSPP/linux/issues/87
You also need to be a bit careful with existing code because there are
places which do things like:
drivers/usb/host/r8a66597-hcd.c
424 kfree(dev);
^^^
425
426 for (port = 0; port < r8a66597->max_root_hub; port++) {
427 if (r8a66597->root_hub[port].dev == dev) {
^^^
428 r8a66597->root_hub[port].dev = NULL;
429 break;
430 }
431 }
Printing the freed pointer in debug code is another thing people do.
regards,
dan carpenter
Re: [PATCH 1/6] drivers: usb: remove usage of list iterator past the loop body
On Mon, Feb 28, 2022 at 10:20:28AM -0800, Joe Perches wrote: > On Mon, 2022-02-28 at 14:24 +0300, Dan Carpenter wrote: > > > a multi-line indent gets curly braces for readability even though > > it's not required by C. And then both sides would get curly braces. > > That's more your personal preference than a coding style guideline. > It's a USB patch. I thought Greg prefered it that way. Greg has some specific style things which he likes and I have adopted and some I pretend not to see. regards, dan carpenter
Re: [PATCH 1/6] drivers: usb: remove usage of list iterator past the loop body
On Mon, Feb 28, 2022 at 01:03:36PM +0100, Jakob Koschel wrote: > >> @@ -954,7 +957,6 @@ net2272_dequeue(struct usb_ep *_ep, struct usb_request > >> *_req) > >>dev_dbg(ep->dev->dev, "unlink (%s) pio\n", _ep->name); > >>net2272_done(ep, req, -ECONNRESET); > >>} > >> - req = NULL; > > > > Another unrelated change. These are all good changes but send them as > > separate patches. > > You are referring to the req = NULL, right? Yes. > > I've changed the use of 'req' in the same function and assumed that I can > just remove the unnecessary statement. But if it's better to do separately > I'll do that. > These are all changes which made me pause during my review to figure out why they were necessary. The line between what is a related part of a patch is a bit vague and some maintainers will ask you to add or subtract from a patch depending on their individual tastes. I don't really have an exact answer, but I felt like this patch needs to be subtracted from. Especially if there is a whole chunk of the patch which can be removed, then to me, that obviously should be in a different patch. regards, dan carpenter
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, Feb 28, 2022 at 12:08:18PM +0100, Jakob Koschel wrote:
> diff --git a/drivers/scsi/scsi_transport_sas.c
> b/drivers/scsi/scsi_transport_sas.c
> index 4ee578b181da..a8cbd90db9d2 100644
> --- a/drivers/scsi/scsi_transport_sas.c
> +++ b/drivers/scsi/scsi_transport_sas.c
> @@ -1060,26 +1060,29 @@ EXPORT_SYMBOL(sas_port_get_phy);
> * connected to a remote device is a port, so ports must be formed on
> * all devices with phys if they're connected to anything.
> */
> -void sas_port_add_phy(struct sas_port *port, struct sas_phy *phy)
> +void sas_port_add_phy(struct sas_port *port, struct sas_phy *_phy)
_phy is an unfortunate name.
> {
> mutex_lock(>phy_list_mutex);
> - if (unlikely(!list_empty(>port_siblings))) {
> + if (unlikely(!list_empty(&_phy->port_siblings))) {
> /* make sure we're already on this port */
> + struct sas_phy *phy = NULL;
Maybe call this port_phy?
> struct sas_phy *tmp;
>
> list_for_each_entry(tmp, >phy_list, port_siblings)
> - if (tmp == phy)
> + if (tmp == _phy) {
> + phy = tmp;
> break;
> + }
> /* If this trips, you added a phy that was already
>* part of a different port */
> - if (unlikely(tmp != phy)) {
> + if (unlikely(!phy)) {
> dev_printk(KERN_ERR, >dev, "trying to add phy %s
> fails: it's already part of another port\n",
> -dev_name(>dev));
> +dev_name(&_phy->dev));
> BUG();
> }
> } else {
> - sas_port_create_link(port, phy);
> - list_add_tail(>port_siblings, >phy_list);
> + sas_port_create_link(port, _phy);
> + list_add_tail(&_phy->port_siblings, >phy_list);
> port->num_phys++;
> }
> mutex_unlock(>phy_list_mutex);
regards,
dan carpenter
Re: [PATCH 6/6] treewide: remove check of list iterator against head past the loop body
On Mon, Feb 28, 2022 at 12:08:22PM +0100, Jakob Koschel wrote:
> diff --git a/drivers/infiniband/hw/hfi1/tid_rdma.c
> b/drivers/infiniband/hw/hfi1/tid_rdma.c
> index 2a7abf7a1f7f..a069847b56aa 100644
> --- a/drivers/infiniband/hw/hfi1/tid_rdma.c
> +++ b/drivers/infiniband/hw/hfi1/tid_rdma.c
> @@ -1239,7 +1239,7 @@ static int kern_alloc_tids(struct tid_rdma_flow *flow)
> struct hfi1_ctxtdata *rcd = flow->req->rcd;
> struct hfi1_devdata *dd = rcd->dd;
> u32 ngroups, pageidx = 0;
> - struct tid_group *group = NULL, *used;
> + struct tid_group *group = NULL, *used, *tmp;
> u8 use;
>
> flow->tnode_cnt = 0;
> @@ -1248,13 +1248,15 @@ static int kern_alloc_tids(struct tid_rdma_flow *flow)
> goto used_list;
>
> /* First look at complete groups */
> - list_for_each_entry(group, >tid_group_list.list, list) {
> - kern_add_tid_node(flow, rcd, "complete groups", group,
> - group->size);
> + list_for_each_entry(tmp, >tid_group_list.list, list) {
> + kern_add_tid_node(flow, rcd, "complete groups", tmp,
> + tmp->size);
>
> - pageidx += group->size;
> - if (!--ngroups)
> + pageidx += tmp->size;
> + if (!--ngroups) {
> + group = tmp;
> break;
> + }
> }
>
> if (pageidx >= flow->npagesets)
> @@ -1277,7 +1279,7 @@ static int kern_alloc_tids(struct tid_rdma_flow *flow)
>* However, if we are at the head, we have reached the end of the
^
>* complete groups list from the first loop above
^^
>*/
Originally this code tested for an open code list_is_head() so the
comment made sense, but it's out of date now. Just delete it.
> - if (group && >list == >tid_group_list.list)
> + if (!group)
> goto bail_eagain;
> group = list_prepare_entry(group, >tid_group_list.list,
> list);
regards,
dan carpenter
Re: [PATCH 3/6] treewide: fix incorrect use to determine if list is empty
On Mon, Feb 28, 2022 at 12:08:19PM +0100, Jakob Koschel wrote: > The list iterator value will *always* be set by list_for_each_entry(). > It is incorrect to assume that the iterator value will be NULL if the > list is empty. > > Instead of checking the pointer it should be checked if > the list is empty. > In acpi_get_pmu_hw_inf() instead of setting the pointer to NULL > on the break, it is set to the correct value and leaving it > NULL if no element was found. > > Signed-off-by: Jakob Koschel > --- > arch/powerpc/sysdev/fsl_gtm.c| 4 ++-- > drivers/media/pci/saa7134/saa7134-alsa.c | 4 ++-- > drivers/perf/xgene_pmu.c | 13 +++-- > 3 files changed, 11 insertions(+), 10 deletions(-) These are all bug fixes. 1) Send them as 3 separate patches. 2) Add Fixes tags. regards, dan carpenter
Re: [PATCH 1/6] drivers: usb: remove usage of list iterator past the loop body
On Mon, Feb 28, 2022 at 12:08:17PM +0100, Jakob Koschel wrote:
> diff --git a/drivers/usb/gadget/udc/at91_udc.c
> b/drivers/usb/gadget/udc/at91_udc.c
> index 9040a0561466..0fd0307bc07b 100644
> --- a/drivers/usb/gadget/udc/at91_udc.c
> +++ b/drivers/usb/gadget/udc/at91_udc.c
> @@ -150,13 +150,14 @@ static void proc_ep_show(struct seq_file *s, struct
> at91_ep *ep)
> if (list_empty (>queue))
> seq_printf(s, "\t(queue empty)\n");
>
> - else list_for_each_entry (req, >queue, queue) {
> - unsignedlength = req->req.actual;
> + else
> + list_for_each_entry(req, >queue, queue) {
> + unsigned intlength = req->req.actual;
>
> - seq_printf(s, "\treq %p len %d/%d buf %p\n",
> - >req, length,
> - req->req.length, req->req.buf);
> - }
> + seq_printf(s, "\treq %p len %d/%d buf %p\n",
> + >req, length,
> + req->req.length, req->req.buf);
> + }
Don't make unrelated white space changes. It just makes the patch
harder to review. As you're writing the patch make note of any
additional changes and do them later in a separate patch.
Also a multi-line indent gets curly braces for readability even though
it's not required by C. And then both sides would get curly braces.
> spin_unlock_irqrestore(>lock, flags);
> }
>
> @@ -226,7 +227,7 @@ static int proc_udc_show(struct seq_file *s, void *unused)
>
> if (udc->enabled && udc->vbus) {
> proc_ep_show(s, >ep[0]);
> - list_for_each_entry (ep, >gadget.ep_list, ep.ep_list) {
> + list_for_each_entry(ep, >gadget.ep_list, ep.ep_list) {
Another unrelated change.
> if (ep->ep.desc)
> proc_ep_show(s, ep);
> }
[ snip ]
> diff --git a/drivers/usb/gadget/udc/net2272.c
> b/drivers/usb/gadget/udc/net2272.c
> index 7c38057dcb4a..bb59200f1596 100644
> --- a/drivers/usb/gadget/udc/net2272.c
> +++ b/drivers/usb/gadget/udc/net2272.c
> @@ -926,7 +926,8 @@ static int
> net2272_dequeue(struct usb_ep *_ep, struct usb_request *_req)
> {
> struct net2272_ep *ep;
> - struct net2272_request *req;
> + struct net2272_request *req = NULL;
> + struct net2272_request *tmp;
> unsigned long flags;
> int stopped;
>
> @@ -939,11 +940,13 @@ net2272_dequeue(struct usb_ep *_ep, struct usb_request
> *_req)
> ep->stopped = 1;
>
> /* make sure it's still queued on this endpoint */
> - list_for_each_entry(req, >queue, queue) {
> - if (>req == _req)
> + list_for_each_entry(tmp, >queue, queue) {
> + if (>req == _req) {
> + req = tmp;
> break;
> + }
> }
> - if (>req != _req) {
> + if (!req) {
> ep->stopped = stopped;
> spin_unlock_irqrestore(>dev->lock, flags);
> return -EINVAL;
> @@ -954,7 +957,6 @@ net2272_dequeue(struct usb_ep *_ep, struct usb_request
> *_req)
> dev_dbg(ep->dev->dev, "unlink (%s) pio\n", _ep->name);
> net2272_done(ep, req, -ECONNRESET);
> }
> - req = NULL;
Another unrelated change. These are all good changes but send them as
separate patches.
> ep->stopped = stopped;
>
> spin_unlock_irqrestore(>dev->lock, flags);
regards,
dan carpenter
Re: [PATCH] powerpc: platforms: 52xx: Fix a resource leak in an error handling path
On Sat, Jan 29, 2022 at 08:16:04AM +0100, Christophe JAILLET wrote:
> The error handling path of mpc52xx_lpbfifo_probe() and a request_irq() is
> not balanced by a corresponding free_irq().
>
> Add the missing call, as already done in the remove function.
>
> Fixes: 3c9059d79f5e ("powerpc/5200: add LocalPlus bus FIFO device driver")
> Signed-off-by: Christophe JAILLET
> ---
> Another strange thing is that the remove function has:
> /* Release the bestcomm transmit task */
> free_irq(bcom_get_task_irq(lpbfifo.bcom_tx_task), );
> but I've not been able to find a corresponding request_irq().
>
> Is it dead code? Is there something missing in the probe?
> (...Is it working?...)
I think you're right that the tx_task IRQ is never allocated.
I'm pretty sure that if you free a zero IRQ then it's a no-op. It won't
find the 0 in the radix tree so irq_to_desc() returns NULL and free_irq()
returns early.
regards,
dan carpenter
Re: WARN_ON() is buggy for 32 bit systems
On Thu, Jan 27, 2022 at 10:10:32PM +1100, Michael Ellerman wrote: > Dan Carpenter writes: > > On Wed, Jan 26, 2022 at 12:21:49PM +, Christophe Leroy wrote: > >> The code is enclosed in a #ifdef CONFIG_PPC64, it is not used for PPC32: > >> > >> /arch/powerpc/include/asm/bug.h > >>99 #ifdef CONFIG_PPC64 > > > > Ah... > > > > You know, life would be a lot easier for me personally if we added an > > #ifndef __CHECKER__ as well... I can't compile PowerPC code so I can't > > test a patch like that. > > Ubuntu & Fedora both have cross compilers packaged, or there's cross > compilers on kernel.org. But I assume you mean you'd rather not bother > compiling for powerpc, which is fair enough. > > Do you mean something like below? Yes, please. > > I'm not sure about that, as it would prevent sparse from checking the > actual BUG_ON code we're using, vs the generic version which we never > use on 64-bit. Is there a smatch specific macro we could check? There isn't a Smatch define. This shouldn't affect Sparse at all unless there was a bug in the WARN_ON() macro. regards, dan carpenter
Re: WARN_ON() is buggy for 32 bit systems
On Wed, Jan 26, 2022 at 12:21:49PM +, Christophe Leroy wrote: > The code is enclosed in a #ifdef CONFIG_PPC64, it is not used for PPC32: > > /arch/powerpc/include/asm/bug.h >99 #ifdef CONFIG_PPC64 Ah... You know, life would be a lot easier for me personally if we added an #ifndef __CHECKER__ as well... I can't compile PowerPC code so I can't test a patch like that. regards, dan carpenter
WARN_ON() is buggy for 32 bit systems
Hi Michael,
Commit e432fe97f3e5 ("powerpc/bug: Cast to unsigned long before passing
to inline asm") breaks WARN_ON() for 32 bit systems.
arch/powerpc/include/asm/bug.h
109 #define WARN_ON(x) ({ \
110 bool __ret_warn_on = false; \
111 do {\
112 if (__builtin_constant_p((x))) {\
113 if (!(x)) \
114 break; \
115 __WARN(); \
116 __ret_warn_on = true; \
117 } else {\
118 __label__ __label_warn_on; \
119 \
120 WARN_ENTRY(PPC_TLNEI " %4, 0", \
121 BUGFLAG_WARNING |
BUGFLAG_TAINT(TAINT_WARN), \
122 __label_warn_on, \
123 "r" ((__force long)(x)));\
If the code is "if (WARN_ON(some_u64)) {" then the cast to long will
truncate away the high bits so it's wrong. (Or at least that's how it
works on x86, I'm working on a work around for Smatch to be able to
parse this WARN_ON(). I don't know anything about PowerPC.)
124 break; \
125 __label_warn_on:\
126 __ret_warn_on = true; \
127 } \
128 } while (0);\
129 unlikely(__ret_warn_on); \
130 })
regards,
dan carpenter
[bug report] soc: fsl: qe: convert QE interrupt controller to platform_device
Hello Maxim Kochetkov,
The patch be7ecbd240b2: "soc: fsl: qe: convert QE interrupt
controller to platform_device" from Aug 3, 2021, leads to the
following static checker warning:
drivers/soc/fsl/qe/qe_ic.c:438 qe_ic_init()
warn: unsigned 'qe_ic->virq_low' is never less than zero.
drivers/soc/fsl/qe/qe_ic.c
408 static int qe_ic_init(struct platform_device *pdev)
409 {
410 struct device *dev = >dev;
411 void (*low_handler)(struct irq_desc *desc);
412 void (*high_handler)(struct irq_desc *desc);
413 struct qe_ic *qe_ic;
414 struct resource *res;
415 struct device_node *node = pdev->dev.of_node;
416
417 res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
418 if (res == NULL) {
419 dev_err(dev, "no memory resource defined\n");
420 return -ENODEV;
421 }
422
423 qe_ic = devm_kzalloc(dev, sizeof(*qe_ic), GFP_KERNEL);
424 if (qe_ic == NULL)
425 return -ENOMEM;
426
427 qe_ic->regs = devm_ioremap(dev, res->start, resource_size(res));
428 if (qe_ic->regs == NULL) {
429 dev_err(dev, "failed to ioremap() registers\n");
430 return -ENODEV;
431 }
432
433 qe_ic->hc_irq = qe_ic_irq_chip;
434
435 qe_ic->virq_high = platform_get_irq(pdev, 0);
436 qe_ic->virq_low = platform_get_irq(pdev, 1);
437
--> 438 if (qe_ic->virq_low < 0) {
439 return -ENODEV;
440 }
Unsigned can't be less than zero. It's weird that it doesn't check
qe_ic->virq_high as well. Also remove the curly braces to make
checkpatch happy?
441
442 if (qe_ic->virq_high != qe_ic->virq_low) {
443 low_handler = qe_ic_cascade_low;
444 high_handler = qe_ic_cascade_high;
445 } else {
446 low_handler = qe_ic_cascade_muxed_mpic;
447 high_handler = NULL;
448 }
449
450 qe_ic->irqhost = irq_domain_add_linear(node, NR_QE_IC_INTS,
451_ic_host_ops, qe_ic);
452 if (qe_ic->irqhost == NULL) {
453 dev_err(dev, "failed to add irq domain\n");
454 return -ENODEV;
455 }
456
457 qe_ic_write(qe_ic->regs, QEIC_CICR, 0);
458
459 irq_set_handler_data(qe_ic->virq_low, qe_ic);
460 irq_set_chained_handler(qe_ic->virq_low, low_handler);
461
462 if (qe_ic->virq_high && qe_ic->virq_high != qe_ic->virq_low) {
463 irq_set_handler_data(qe_ic->virq_high, qe_ic);
464 irq_set_chained_handler(qe_ic->virq_high, high_handler);
465 }
466 return 0;
467 }
regards,
dan carpenter
Re: [PATCH] soc: fsl: qe: convert QE interrupt controller to platform_device
Hi Maxim,
url:
https://github.com/0day-ci/linux/commits/Maxim-Kochetkov/soc-fsl-qe-convert-QE-interrupt-controller-to-platform_device/20210705-191227
base: https://git.kernel.org/pub/scm/linux/kernel/git/soc/soc.git for-next
config: openrisc-randconfig-m031-20210709 (attached as .config)
compiler: or1k-linux-gcc (GCC) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
Reported-by: Dan Carpenter
smatch warnings:
drivers/soc/fsl/qe/qe_ic.c:461 qe_ic_init() warn: 'qe_ic->regs' not released on
lines: 442.
vim +461 drivers/soc/fsl/qe/qe_ic.c
43f09464f68dbb drivers/soc/fsl/qe/qe_ic.c Maxim Kochetkov 2021-07-05
408 static int qe_ic_init(struct platform_device *pdev)
9865853851313e arch/powerpc/sysdev/qe_lib/qe_ic.c Li Yang 2006-10-03
409 {
4e0e161d3cc403 drivers/soc/fsl/qe/qe_ic.c Rasmus Villemoes 2019-11-28
410 void (*low_handler)(struct irq_desc *desc);
4e0e161d3cc403 drivers/soc/fsl/qe/qe_ic.c Rasmus Villemoes 2019-11-28
411 void (*high_handler)(struct irq_desc *desc);
9865853851313e arch/powerpc/sysdev/qe_lib/qe_ic.c Li Yang 2006-10-03
412 struct qe_ic *qe_ic;
9865853851313e arch/powerpc/sysdev/qe_lib/qe_ic.c Li Yang 2006-10-03
413 struct resource res;
43f09464f68dbb drivers/soc/fsl/qe/qe_ic.c Maxim Kochetkov 2021-07-05
414 struct device_node *node = pdev->dev.of_node;
882c626d1d4650 drivers/soc/fsl/qe/qe_ic.c Rasmus Villemoes 2019-11-28
415 u32 ret;
9865853851313e arch/powerpc/sysdev/qe_lib/qe_ic.c Li Yang 2006-10-03
416
2272a55f16c998 arch/powerpc/sysdev/qe_lib/qe_ic.c Michael Ellerman 2008-05-26
417 ret = of_address_to_resource(node, 0, );
2272a55f16c998 arch/powerpc/sysdev/qe_lib/qe_ic.c Michael Ellerman 2008-05-26
418 if (ret)
43f09464f68dbb drivers/soc/fsl/qe/qe_ic.c Maxim Kochetkov 2021-07-05
419 return -ENODEV;
2272a55f16c998 arch/powerpc/sysdev/qe_lib/qe_ic.c Michael Ellerman 2008-05-26
420
ea96025a26ab89 arch/powerpc/sysdev/qe_lib/qe_ic.c Anton Vorontsov 2009-07-01
421 qe_ic = kzalloc(sizeof(*qe_ic), GFP_KERNEL);
9865853851313e arch/powerpc/sysdev/qe_lib/qe_ic.c Li Yang 2006-10-03
422 if (qe_ic == NULL)
43f09464f68dbb drivers/soc/fsl/qe/qe_ic.c Maxim Kochetkov 2021-07-05
423 return -ENOMEM;
9865853851313e arch/powerpc/sysdev/qe_lib/qe_ic.c Li Yang 2006-10-03
424
a8db8cf0d894df arch/powerpc/sysdev/qe_lib/qe_ic.c Grant Likely 2012-02-14
425 qe_ic->irqhost = irq_domain_add_linear(node, NR_QE_IC_INTS,
a8db8cf0d894df arch/powerpc/sysdev/qe_lib/qe_ic.c Grant Likely 2012-02-14
426 _ic_host_ops, qe_ic);
3475dd8a68a7c7 arch/powerpc/sysdev/qe_lib/qe_ic.c Julia Lawall 2009-08-01
427 if (qe_ic->irqhost == NULL) {
^^
Does this need to be cleaned up?
3475dd8a68a7c7 arch/powerpc/sysdev/qe_lib/qe_ic.c Julia Lawall 2009-08-01
428 kfree(qe_ic);
43f09464f68dbb drivers/soc/fsl/qe/qe_ic.c Maxim Kochetkov 2021-07-05
429 return -ENODEV;
3475dd8a68a7c7 arch/powerpc/sysdev/qe_lib/qe_ic.c Julia Lawall 2009-08-01
430 }
9865853851313e arch/powerpc/sysdev/qe_lib/qe_ic.c Li Yang 2006-10-03
431
28f65c11f2ffb3 arch/powerpc/sysdev/qe_lib/qe_ic.c Joe Perches 2011-06-09
432 qe_ic->regs = ioremap(res.start, resource_size());
^
9865853851313e arch/powerpc/sysdev/qe_lib/qe_ic.c Li Yang 2006-10-03
433
9865853851313e arch/powerpc/sysdev/qe_lib/qe_ic.c Li Yang 2006-10-03
434 qe_ic->hc_irq = qe_ic_irq_chip;
9865853851313e arch/powerpc/sysdev/qe_lib/qe_ic.c Li Yang 2006-10-03
435
9865853851313e arch/powerpc/sysdev/qe_lib/qe_ic.c Li Yang 2006-10-03
436 qe_ic->virq_high = irq_of_parse_and_map(node, 0);
9865853851313e arch/powerpc/sysdev/qe_lib/qe_ic.c Li Yang 2006-10-03
437 qe_ic->virq_low = irq_of_parse_and_map(node, 1);
9865853851313e arch/powerpc/sysdev/qe_lib/qe_ic.c Li Yang 2006-10-03
438
10d7930dbb51a8 drivers/soc/fsl/qe/qe_ic.c Rasmus Villemoes 2019-11-28
439 if (!qe_ic->virq_low) {
9865853851313e arch/powerpc/sysdev/qe_lib/qe_ic.c Li Yang 2006-10-03
440 printk(KERN_ERR "Failed to map QE_IC low IRQ\n");
3475dd8a68a7c7 arch/powerpc/sysdev/qe_lib/qe_ic.c Julia Lawall 2009-08-01
441 kfree(qe_ic);
43f09464f68dbb drivers/soc/fsl/qe/qe_ic.c Maxim Kochetkov 2021-07-05
442 return -ENODEV;
Call iounmap() before returning?
9865853851313e arch/powerpc/sysdev/qe_lib/qe_ic.c Li Yang 2
Re: [PATCH -next 2/3] xen: balloon: Replaced simple_strtoull() with kstrtoull()
On Thu, May 27, 2021 at 02:10:21PM +, David Laight wrote: > From: Chen Huang > > Sent: 26 May 2021 10:20 > > > > The simple_strtoull() function is deprecated in some situation, since > > it does not check for the range overflow, use kstrtoull() instead. > > > ... > > - target_bytes = simple_strtoull(buf, , 0) * 1024; > > + ret = kstrtoull(buf, 0, _bytes); > > + if (ret) > > + return ret; > > + target_bytes *= 1024; > > I'd have thought it was more important to check *endchar > than overflow. That's one of the differences between simple_strtoull() and kstrtoull(). The simple_strtoull() will accept a string like "123ABC", but kstrtoull() will only accept NUL terminated numbers or a newline followed by a NUL terminator. Which is fine in this context because users will be doing "echo 1234 > /sys/foo". > If you are worried about overflow you need a range check > before the multiply. This is probably a case where if the users cause an integer overflow then they get what they deserve. regards, dan carpenter
Re: [PATCH] powerpc: Initialize local variable fdt to NULL in elf64_load()
On Thu, Apr 22, 2021 at 08:05:27AM +, David Laight wrote: > From: Daniel Axtens > > Sent: 22 April 2021 03:21 > > > > > Hi Lakshmi, > > > > > >> On 4/15/21 12:14 PM, Lakshmi Ramasubramanian wrote: > > >> > > >> Sorry - missed copying device-tree and powerpc mailing lists. > > >> > > >>> There are a few "goto out;" statements before the local variable "fdt" > > >>> is initialized through the call to of_kexec_alloc_and_setup_fdt() in > > >>> elf64_load(). This will result in an uninitialized "fdt" being passed > > >>> to kvfree() in this function if there is an error before the call to > > >>> of_kexec_alloc_and_setup_fdt(). > > >>> > > >>> Initialize the local variable "fdt" to NULL. > > >>> > > > I'm a huge fan of initialising local variables! But I'm struggling to > > > find the code path that will lead to an uninit fdt being returned... > > > > OK, so perhaps this was putting it too strongly. I have been bitten > > by uninitialised things enough in C that I may have taken a slightly > > overly-agressive view of fixing them in the source rather than the > > compiler. I do think compiler-level mitigations are better, and I take > > the point that we don't want to defeat compiler checking. > > > > (Does anyone - and by anyone I mean any large distro - compile with > > local variables inited by the compiler?) > > There are compilers that initialise locals to zero for 'debug' builds > and leave the 'random' for optimised 'release' builds. > Lets not test what we are releasing! We're eventually going to move to a world where initializing to zero it the default for the kernel. I think people will still want to initialize to a poison value for debug builds. Initializing to zero is better for debugging because it's more predictable. An it avoid information leaks. And dereferencing random uninitialized pointers is a privilege escalation but dereferencing a NULL is just an Oops. The speed impact is not very significant because (conceptually) it only needs to be done where there is a compiler warning about uninitialized variables. It's slightly more complicated in real life. In this case, the compiler doesn't know what happens inside the kexec_build_elf_info() function so it silences the warning. And GCC silences warnings if the variable is initialized inside a loop even when it doesn't know that we enter the loop. regards, dan carpenter
Re: [RFC v1 PATCH 1/3] drivers: soc: add support for soc_device_match returning -EPROBE_DEFER
On Mon, Apr 19, 2021 at 10:20:13AM +0200, Geert Uytterhoeven wrote:
> Hi Alice,
>
> CC Arnd (soc_device_match() author)
>
> On Mon, Apr 19, 2021 at 6:28 AM Alice Guo (OSS) wrote:
> > From: Alice Guo
> >
> > In i.MX8M boards, the registration of SoC device is later than caam
> > driver which needs it. Caam driver needs soc_device_match to provide
> > -EPROBE_DEFER when no SoC device is registered and no
> > early_soc_dev_attr.
>
> I'm wondering if this is really a good idea: soc_device_match() is a
> last-resort low-level check, and IMHO should be made available early on,
> so there is no need for -EPROBE_DEFER.
>
> >
> > Signed-off-by: Alice Guo
>
> Thanks for your patch!
>
> > --- a/drivers/base/soc.c
> > +++ b/drivers/base/soc.c
> > @@ -110,6 +110,7 @@ static void soc_release(struct device *dev)
> > }
> >
> > static struct soc_device_attribute *early_soc_dev_attr;
> > +static bool soc_dev_attr_init_done = false;
>
> Do you need this variable?
>
> >
> > struct soc_device *soc_device_register(struct soc_device_attribute
> > *soc_dev_attr)
> > {
> > @@ -157,6 +158,7 @@ struct soc_device *soc_device_register(struct
> > soc_device_attribute *soc_dev_attr
> > return ERR_PTR(ret);
> > }
> >
> > + soc_dev_attr_init_done = true;
> > return soc_dev;
> >
> > out3:
> > @@ -246,6 +248,9 @@ const struct soc_device_attribute *soc_device_match(
> > if (!matches)
> > return NULL;
> >
> > + if (!soc_dev_attr_init_done && !early_soc_dev_attr)
>
> if (!soc_bus_type.p && !early_soc_dev_attr)
There is one place checking this already. We could wrap it in a helper
function:
static bool device_init_done(void)
{
return soc_bus_type.p ? true : false;
}
regards,
dan carpenter
Re: [PATCH] powerpc: Initialize local variable fdt to NULL in elf64_load()
On Tue, Apr 20, 2021 at 09:30:16AM +1000, Michael Ellerman wrote:
> Lakshmi Ramasubramanian writes:
> > On 4/16/21 2:05 AM, Michael Ellerman wrote:
> >
> >> Daniel Axtens writes:
> >>>> On 4/15/21 12:14 PM, Lakshmi Ramasubramanian wrote:
> >>>>
> >>>> Sorry - missed copying device-tree and powerpc mailing lists.
> >>>>
> >>>>> There are a few "goto out;" statements before the local variable "fdt"
> >>>>> is initialized through the call to of_kexec_alloc_and_setup_fdt() in
> >>>>> elf64_load(). This will result in an uninitialized "fdt" being passed
> >>>>> to kvfree() in this function if there is an error before the call to
> >>>>> of_kexec_alloc_and_setup_fdt().
> >>>>>
> >>>>> Initialize the local variable "fdt" to NULL.
> >>>>>
> >>> I'm a huge fan of initialising local variables! But I'm struggling to
> >>> find the code path that will lead to an uninit fdt being returned...
> >>>
> >>> The out label reads in part:
> >>>
> >>> /* Make kimage_file_post_load_cleanup free the fdt buffer for us. */
> >>> return ret ? ERR_PTR(ret) : fdt;
> >>>
> >>> As far as I can tell, any time we get a non-zero ret, we're going to
> >>> return an error pointer rather than the uninitialised value...
> >
> > As Dan pointed out, the new code is in linux-next.
> >
> > I have copied the new one below - the function doesn't return fdt, but
> > instead sets it in the arch specific field (please see the link to the
> > updated elf_64.c below).
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git/tree/arch/powerpc/kexec/elf_64.c?h=for-next
> >
> >
> >>>
> >>> (btw, it does look like we might leak fdt if we have an error after we
> >>> successfully kmalloc it.)
> >>>
> >>> Am I missing something? Can you link to the report for the kernel test
> >>> robot or from Dan?
> >
> > /*
> > * Once FDT buffer has been successfully passed to
> > kexec_add_buffer(),
> > * the FDT buffer address is saved in image->arch.fdt. In that
> > case,
> > * the memory cannot be freed here in case of any other error.
> > */
> > if (ret && !image->arch.fdt)
> > kvfree(fdt);
> >
> > return ret ? ERR_PTR(ret) : NULL;
> >
> > In case of an error, the memory allocated for fdt is freed unless it has
> > already been passed to kexec_add_buffer().
>
> It feels like the root of the problem is that the kvfree of fdt is in
> the wrong place. It's only allocated later in the function, so the error
> path should reflect that. Something like the patch below.
>
> cheers
>
>
> diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c
> index 5a569bb51349..02662e72c53d 100644
> --- a/arch/powerpc/kexec/elf_64.c
> +++ b/arch/powerpc/kexec/elf_64.c
> @@ -114,7 +114,7 @@ static void *elf64_load(struct kimage *image, char
> *kernel_buf,
> ret = setup_new_fdt_ppc64(image, fdt, initrd_load_addr,
> initrd_len, cmdline);
> if (ret)
> - goto out;
> + goto out_free_fdt;
>
> fdt_pack(fdt);
>
> @@ -125,7 +125,7 @@ static void *elf64_load(struct kimage *image, char
> *kernel_buf,
> kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
> ret = kexec_add_buffer();
> if (ret)
> - goto out;
> + goto out_free_fdt;
>
> /* FDT will be freed in arch_kimage_file_post_load_cleanup */
> image->arch.fdt = fdt;
> @@ -140,18 +140,14 @@ static void *elf64_load(struct kimage *image, char
> *kernel_buf,
> if (ret)
> pr_err("Error setting up the purgatory.\n");
>
> + goto out;
This will leak. It would need to be something like:
if (ret) {
pr_err("Error setting up the purgatory.\n");
goto out_free_fdt;
}
goto out;
But we should also fix the uninitialized variable of "elf_info" if
kexec_build_elf_info() fails.
> +
> +out_free_fdt:
> + kvfree(fdt);
> out:
> kfree(modified_cmdline);
> kexec_free_elf_info(_info);
>
> - /*
> - * Once FDT buffer has been successfully passed to kexec_add_buffer(),
> - * the FDT buffer address is saved in image->arch.fdt. In that case,
> - * the memory cannot be freed here in case of any other error.
> - */
> - if (ret && !image->arch.fdt)
> - kvfree(fdt);
> -
> return ret ? ERR_PTR(ret) : NULL;
> }
regards,
dan carpenter
Re: [PATCH] powerpc: Initialize local variable fdt to NULL in elf64_load()
On Fri, Apr 16, 2021 at 09:00:12AM +0200, Christophe Leroy wrote:
>
>
> Le 16/04/2021 à 08:44, Daniel Axtens a écrit :
> > Hi Lakshmi,
> >
> > > On 4/15/21 12:14 PM, Lakshmi Ramasubramanian wrote:
> > >
> > > Sorry - missed copying device-tree and powerpc mailing lists.
> > >
> > > > There are a few "goto out;" statements before the local variable "fdt"
> > > > is initialized through the call to of_kexec_alloc_and_setup_fdt() in
> > > > elf64_load(). This will result in an uninitialized "fdt" being passed
> > > > to kvfree() in this function if there is an error before the call to
> > > > of_kexec_alloc_and_setup_fdt().
> > > >
> > > > Initialize the local variable "fdt" to NULL.
> > > >
> > I'm a huge fan of initialising local variables! But I'm struggling to
> > find the code path that will lead to an uninit fdt being returned...
> >
> > The out label reads in part:
> >
> > /* Make kimage_file_post_load_cleanup free the fdt buffer for us. */
> > return ret ? ERR_PTR(ret) : fdt;
> >
> > As far as I can tell, any time we get a non-zero ret, we're going to
> > return an error pointer rather than the uninitialised value...
>
> I don't think GCC is smart enough to detect that.
>
We disabled uninitialized variable checking for GCC.
But actually is something that has been on my mind recently. Smatch is
supposed to parse this correctly but there is a bug that affects powerpc
and I don't know how to debug it. The kbuild bot is doing cross
platform compiles but I don't have one set up on myself. Could someone
with Smatch installed test something for me?
Or if you don't have Smatch installed then you should definitely install
it. :P
https://www.spinics.net/lists/smatch/msg00568.html
Apply the patch from below and edit the path to point to the correct
directory. Then run kchecker and email me the output?
~/path/to/smatch_scripts/kchecker arch/powerpc/kernel/hw_breakpoint.c
regads,
dan carpenter
diff --git a/arch/powerpc/kernel/hw_breakpoint.c
b/arch/powerpc/kernel/hw_breakpoint.c
index 8fc7a14e4d71..f2dfba54e14d 100644
--- a/arch/powerpc/kernel/hw_breakpoint.c
+++ b/arch/powerpc/kernel/hw_breakpoint.c
@@ -167,13 +167,19 @@ static bool can_co_exist(struct breakpoint *b, struct
perf_event *bp)
return !(alternate_infra_bp(b, bp) && bp_addr_range_overlap(b->bp, bp));
}
+#include "/home/XXX/path/to/smatch/check_debug.h"
static int task_bps_add(struct perf_event *bp)
{
struct breakpoint *tmp;
tmp = alloc_breakpoint(bp);
- if (IS_ERR(tmp))
+ __smatch_about(tmp);
+ __smatch_debug_on();
+ if (IS_ERR(tmp)) {
+ __smatch_debug_off();
+ __smatch_about(tmp);
return PTR_ERR(tmp);
+ }
list_add(>list, _bps);
return 0;
Re: [PATCH] powerpc: Initialize local variable fdt to NULL in elf64_load()
On Fri, Apr 16, 2021 at 04:44:30PM +1000, Daniel Axtens wrote:
> Hi Lakshmi,
>
> > On 4/15/21 12:14 PM, Lakshmi Ramasubramanian wrote:
> >
> > Sorry - missed copying device-tree and powerpc mailing lists.
> >
> >> There are a few "goto out;" statements before the local variable "fdt"
> >> is initialized through the call to of_kexec_alloc_and_setup_fdt() in
> >> elf64_load(). This will result in an uninitialized "fdt" being passed
> >> to kvfree() in this function if there is an error before the call to
> >> of_kexec_alloc_and_setup_fdt().
> >>
> >> Initialize the local variable "fdt" to NULL.
> >>
> I'm a huge fan of initialising local variables!
Don't be! It just disables static checker warnings and hides bugs.
The kbuild emails are archived but the email is mangled and unreadable.
https://www.mail-archive.com/kbuild@lists.01.org/msg06371.html
I think maybe you're not on the most recent code. In linux-next this
code looks like:
arch/powerpc/kexec/elf_64.c
27 static void *elf64_load(struct kimage *image, char *kernel_buf,
28 unsigned long kernel_len, char *initrd,
29 unsigned long initrd_len, char *cmdline,
30 unsigned long cmdline_len)
31 {
32 int ret;
33 unsigned long kernel_load_addr;
34 unsigned long initrd_load_addr = 0, fdt_load_addr;
35 void *fdt;
36 const void *slave_code;
37 struct elfhdr ehdr;
38 char *modified_cmdline = NULL;
39 struct kexec_elf_info elf_info;
40 struct kexec_buf kbuf = { .image = image, .buf_min = 0,
41.buf_max = ppc64_rma_size };
42 struct kexec_buf pbuf = { .image = image, .buf_min = 0,
43.buf_max = ppc64_rma_size, .top_down
= true,
44.mem = KEXEC_BUF_MEM_UNKNOWN };
45
46 ret = kexec_build_elf_info(kernel_buf, kernel_len, ,
_info);
47 if (ret)
48 goto out;
I really despise "goto out;" because freeing things which haven't been
allocated is always dangerous.
[ snip ].
143 out:
144 kfree(modified_cmdline);
145 kexec_free_elf_info(_info);
There is a possibility that "elf_info" has holds uninitialized stack
data if elf_read_ehdr() fails so that's probably fixing as well. kexec()
is root only so this can't be exploited.
146
147 /*
148 * Once FDT buffer has been successfully passed to
kexec_add_buffer(),
149 * the FDT buffer address is saved in image->arch.fdt. In that
case,
150 * the memory cannot be freed here in case of any other error.
151 */
152 if (ret && !image->arch.fdt)
153 kvfree(fdt);
^^^
Uninitialized.
154
155 return ret ? ERR_PTR(ret) : NULL;
156 }
regards,
dan carpenter
Re: [PATCH net-next v2 1/2] of: net: pass the dst buffer to of_get_mac_address()
Hi Michael,
url:
https://github.com/0day-ci/linux/commits/Michael-Walle/of-net-support-non-platform-devices-in-of_get_mac_address/20210406-234030
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git
cc0626c2aaed8e475efdd85fa374b497a7192e35
config: x86_64-randconfig-m001-20210406 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
Reported-by: Dan Carpenter
smatch warnings:
drivers/net/ethernet/xilinx/xilinx_axienet_main.c:2069 axienet_probe() warn:
passing a valid pointer to 'PTR_ERR'
vim +/PTR_ERR +2069 drivers/net/ethernet/xilinx/xilinx_axienet_main.c
522856cefaf09d Robert Hancock 2019-06-06 2060 /* Check for Ethernet
core IRQ (optional) */
522856cefaf09d Robert Hancock 2019-06-06 2061 if (lp->eth_irq <= 0)
522856cefaf09d Robert Hancock 2019-06-06 2062
dev_info(>dev, "Ethernet core IRQ not defined\n");
522856cefaf09d Robert Hancock 2019-06-06 2063
8a3b7a252dca9f Daniel Borkmann 2012-01-19 2064 /* Retrieve the MAC
address */
411b125c6ace1f Michael Walle 2021-04-06 2065 ret =
of_get_mac_address(pdev->dev.of_node, mac_addr);
411b125c6ace1f Michael Walle 2021-04-06 2066 if (!ret) {
411b125c6ace1f Michael Walle 2021-04-06 2067
axienet_set_mac_address(ndev, mac_addr);
411b125c6ace1f Michael Walle 2021-04-06 2068 } else {
d05a9ed5c3a773 Robert Hancock 2019-06-06 @2069
dev_warn(>dev, "could not find MAC address property: %ld\n",
d05a9ed5c3a773 Robert Hancock 2019-06-06 2070
PTR_ERR(mac_addr));
^
This should print "ret".
411b125c6ace1f Michael Walle 2021-04-06 2071
axienet_set_mac_address(ndev, NULL);
8a3b7a252dca9f Daniel Borkmann 2012-01-19 2072 }
8a3b7a252dca9f Daniel Borkmann 2012-01-19 2073
8a3b7a252dca9f Daniel Borkmann 2012-01-19 2074 lp->coalesce_count_rx =
XAXIDMA_DFT_RX_THRESHOLD;
8a3b7a252dca9f Daniel Borkmann 2012-01-19 2075 lp->coalesce_count_tx =
XAXIDMA_DFT_TX_THRESHOLD;
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
.config.gz
Description: application/gzip
[kbuild] Re: [PATCH net-next v2 1/2] of: net: pass the dst buffer to of_get_mac_address()
Hi Michael,
I love your patch! Perhaps something to improve:
[auto build test WARNING on net-next/master]
url:
https://github.com/0day-ci/linux/commits/Michael-Walle/of-net-support-non-platform-devices-in-of_get_mac_address/20210406-234030
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git
cc0626c2aaed8e475efdd85fa374b497a7192e35
config: x86_64-randconfig-m001-20210406 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
Reported-by: Dan Carpenter
smatch warnings:
drivers/net/ethernet/xilinx/xilinx_axienet_main.c:2069 axienet_probe() warn:
passing a valid pointer to 'PTR_ERR'
vim +/PTR_ERR +2069 drivers/net/ethernet/xilinx/xilinx_axienet_main.c
2be586205ca2b8 Srikanth Thokala2015-05-05 1832 static int
axienet_probe(struct platform_device *pdev)
8a3b7a252dca9f Daniel Borkmann 2012-01-19 1833 {
8495659bf93c8e Srikanth Thokala2015-05-05 1834 int ret;
8a3b7a252dca9f Daniel Borkmann 2012-01-19 1835 struct device_node *np;
8a3b7a252dca9f Daniel Borkmann 2012-01-19 1836 struct axienet_local
*lp;
8a3b7a252dca9f Daniel Borkmann 2012-01-19 1837 struct net_device *ndev;
28ef9ebdb64c6f Robert Hancock 2019-06-06 1838 struct resource *ethres;
411b125c6ace1f Michael Walle 2021-04-06 1839 u8 mac_addr[ETH_ALEN];
^^
5fff0151b3244d Andre Przywara 2020-03-24 1840 int addr_width = 32;
8495659bf93c8e Srikanth Thokala2015-05-05 1841 u32 value;
8a3b7a252dca9f Daniel Borkmann 2012-01-19 1842
8a3b7a252dca9f Daniel Borkmann 2012-01-19 1843 ndev =
alloc_etherdev(sizeof(*lp));
41de8d4cff21a2 Joe Perches 2012-01-29 1844 if (!ndev)
8a3b7a252dca9f Daniel Borkmann 2012-01-19 1845 return -ENOMEM;
8a3b7a252dca9f Daniel Borkmann 2012-01-19 1846
95219aa538e11d Srikanth Thokala2015-05-05 1847
platform_set_drvdata(pdev, ndev);
8a3b7a252dca9f Daniel Borkmann 2012-01-19 1848
95219aa538e11d Srikanth Thokala2015-05-05 1849 SET_NETDEV_DEV(ndev,
>dev);
8a3b7a252dca9f Daniel Borkmann 2012-01-19 1850 ndev->flags &=
~IFF_MULTICAST; /* clear multicast */
28e24c62ab3062 Eric Dumazet2013-12-02 1851 ndev->features =
NETIF_F_SG;
8a3b7a252dca9f Daniel Borkmann 2012-01-19 1852 ndev->netdev_ops =
_netdev_ops;
8a3b7a252dca9f Daniel Borkmann 2012-01-19 1853 ndev->ethtool_ops =
_ethtool_ops;
8a3b7a252dca9f Daniel Borkmann 2012-01-19 1854
d894be57ca92c8 Jarod Wilson2016-10-20 1855 /* MTU range: 64 - 9000
*/
d894be57ca92c8 Jarod Wilson2016-10-20 1856 ndev->min_mtu = 64;
d894be57ca92c8 Jarod Wilson2016-10-20 1857 ndev->max_mtu =
XAE_JUMBO_MTU;
d894be57ca92c8 Jarod Wilson2016-10-20 1858
8a3b7a252dca9f Daniel Borkmann 2012-01-19 1859 lp = netdev_priv(ndev);
8a3b7a252dca9f Daniel Borkmann 2012-01-19 1860 lp->ndev = ndev;
95219aa538e11d Srikanth Thokala2015-05-05 1861 lp->dev = >dev;
8a3b7a252dca9f Daniel Borkmann 2012-01-19 1862 lp->options =
XAE_OPTION_DEFAULTS;
8b09ca823ffb4e Robert Hancock 2019-06-06 1863 lp->rx_bd_num =
RX_BD_NUM_DEFAULT;
8b09ca823ffb4e Robert Hancock 2019-06-06 1864 lp->tx_bd_num =
TX_BD_NUM_DEFAULT;
57baf8cc70ea4c Robert Hancock 2021-02-12 1865
b11bfb9a19f9d7 Robert Hancock 2021-03-25 1866 lp->axi_clk =
devm_clk_get_optional(>dev, "s_axi_lite_clk");
b11bfb9a19f9d7 Robert Hancock 2021-03-25 1867 if (!lp->axi_clk) {
b11bfb9a19f9d7 Robert Hancock 2021-03-25 1868 /* For backward
compatibility, if named AXI clock is not present,
b11bfb9a19f9d7 Robert Hancock 2021-03-25 1869 * treat the
first clock specified as the AXI clock.
b11bfb9a19f9d7 Robert Hancock 2021-03-25 1870 */
b11bfb9a19f9d7 Robert Hancock 2021-03-25 1871 lp->axi_clk =
devm_clk_get_optional(>dev, NULL);
b11bfb9a19f9d7 Robert Hancock 2021-03-25 1872 }
b11bfb9a19f9d7 Robert Hancock 2021-03-25 1873 if
(IS_ERR(lp->axi_clk)) {
b11bfb9a19f9d7 Robert Hancock 2021-03-25 1874 ret =
PTR_ERR(lp->axi_clk);
57baf8cc70ea4c Robert Hancock 2021-02-12 1875 goto
free_netdev;
57baf8cc70ea4c Robert Hancock 2021-02-12 1876 }
b11bfb9a19f9d7 Robert Hancock 2021-03-25 1877 ret =
clk_prepare_enable(lp->axi_clk);
57baf8cc70ea4c Robert Hancock 2021-02-12 1878 if (ret) {
b11bfb9a19f9d7 Robert Hancock 2021-03-25 1879
dev_err(>dev, "Unable to enable AXI clock: %d\n", ret);
57baf8cc70ea4c Robert Hancock 2021-02-12 1880 goto
free_netdev;
57baf8cc70ea4c Robe
[PATCH] ASoC: fsl: imx-hdmi: Fix an uninitialized return in probe
The "ret" variable should be set to a negative error code but
currently it returns uninitialized stack data.
Fixes: 6a5f850aa83a ("ASoC: fsl: Add imx-hdmi machine driver")
Signed-off-by: Dan Carpenter
---
sound/soc/fsl/imx-hdmi.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/sound/soc/fsl/imx-hdmi.c b/sound/soc/fsl/imx-hdmi.c
index 2c2a76a71940..ede4a9ad1054 100644
--- a/sound/soc/fsl/imx-hdmi.c
+++ b/sound/soc/fsl/imx-hdmi.c
@@ -164,6 +164,7 @@ static int imx_hdmi_probe(struct platform_device *pdev)
if ((hdmi_out && hdmi_in) || (!hdmi_out && !hdmi_in)) {
dev_err(>dev, "Invalid HDMI DAI link\n");
+ ret = -EINVAL;
goto fail;
}
--
2.29.2
Re: [PATCH v4 2/3] mm, treewide: Rename kzfree() to kfree_sensitive()
Last time you sent this we couldn't decide which tree it should go through. Either the crypto tree or through Andrew seems like the right thing to me. Also the other issue is that it risks breaking things if people add new kzfree() instances while we are doing the transition. Could you just add a "#define kzfree kfree_sensitive" so that things continue to compile and we can remove it in the next kernel release? regards, dan carpenter
Re: [PATCH v4 1/3] mm/slab: Use memzero_explicit() in kzfree()
On Tue, Jun 16, 2020 at 08:42:08AM +0200, Michal Hocko wrote:
> On Mon 15-06-20 21:57:16, Waiman Long wrote:
> > The kzfree() function is normally used to clear some sensitive
> > information, like encryption keys, in the buffer before freeing it back
> > to the pool. Memset() is currently used for the buffer clearing. However,
> > it is entirely possible that the compiler may choose to optimize away the
> > memory clearing especially if LTO is being used. To make sure that this
> > optimization will not happen, memzero_explicit(), which is introduced
> > in v3.18, is now used in kzfree() to do the clearing.
> >
> > Fixes: 3ef0e5ba4673 ("slab: introduce kzfree()")
> > Cc: sta...@vger.kernel.org
> > Signed-off-by: Waiman Long
>
> Acked-by: Michal Hocko
>
> Although I am not really sure this is a stable material. Is there any
> known instance where the memset was optimized out from kzfree?
I told him to add the stable. Otherwise it will just get reported to
me again. It's a just safer to backport it before we forget.
regards,
dan carpenter
Re: [PATCH 1/2] mm, treewide: Rename kzfree() to kfree_sensitive()
On Mon, Apr 13, 2020 at 05:15:49PM -0400, Waiman Long wrote:
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index 23c7500eea7d..c08bc7eb20bd 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -1707,17 +1707,17 @@ void *krealloc(const void *p, size_t new_size, gfp_t
> flags)
> EXPORT_SYMBOL(krealloc);
>
> /**
> - * kzfree - like kfree but zero memory
> + * kfree_sensitive - Clear sensitive information in memory before freeing
> * @p: object to free memory of
> *
> * The memory of the object @p points to is zeroed before freed.
> - * If @p is %NULL, kzfree() does nothing.
> + * If @p is %NULL, kfree_sensitive() does nothing.
> *
> * Note: this function zeroes the whole allocated buffer which can be a good
> * deal bigger than the requested buffer size passed to kmalloc(). So be
> * careful when using this function in performance sensitive code.
> */
> -void kzfree(const void *p)
> +void kfree_sensitive(const void *p)
> {
> size_t ks;
> void *mem = (void *)p;
> @@ -1725,10 +1725,10 @@ void kzfree(const void *p)
> if (unlikely(ZERO_OR_NULL_PTR(mem)))
> return;
> ks = ksize(mem);
> - memset(mem, 0, ks);
> + memzero_explicit(mem, ks);
^
This is an unrelated bug fix. It really needs to be pulled into a
separate patch by itself and back ported to stable kernels.
> kfree(mem);
> }
> -EXPORT_SYMBOL(kzfree);
> +EXPORT_SYMBOL(kfree_sensitive);
>
> /**
> * ksize - get the actual amount of memory allocated for a given object
regards,
dan carpenter
Re: [PATCH] powerpc/nvram: Replace kmalloc with kzalloc in the error message
On Wed, Jun 03, 2020 at 09:37:18PM +1000, Michael Ellerman wrote:
> Dan Carpenter writes:
> > On Tue, Jun 02, 2020 at 09:23:57PM +1000, Michael Ellerman wrote:
> >> Markus Elfring writes:
> >> >>>> Please just remove the message instead, it's a tiny allocation that's
> >> >>>> unlikely to ever fail, and the caller will print an error anyway.
> >> >>>
> >> >>> How do you think about to take another look at a previous update
> >> >>> suggestion
> >> >>> like the following?
> >> >>>
> >> >>> powerpc/nvram: Delete three error messages for a failed memory
> >> >>> allocation
> >> >>> https://patchwork.ozlabs.org/project/linuxppc-dev/patch/00845261-8528-d011-d3b8-e9355a231...@users.sourceforge.net/
> >> >>> https://lore.kernel.org/linuxppc-dev/00845261-8528-d011-d3b8-e9355a231...@users.sourceforge.net/
> >> >>> https://lore.kernel.org/patchwork/patch/752720/
> >> >>> https://lkml.org/lkml/2017/1/19/537
> >> >>
> >> >> That deleted the messages from nvram_scan_partitions(), but neither of
> >> >> the callers of nvram_scan_paritions() check its return value or print
> >> >> anything if it fails. So removing those messages would make those
> >> >> failures silent which is not what we want.
> >> >
> >> > * How do you think about information like the following?
> >> >
> >> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=f359287765c04711ff54fbd11645271d8e5ff763#n883
> >> > “…
> >> > These generic allocation functions all emit a stack dump on failure when
> >> > used
> >> > without __GFP_NOWARN so there is no use in emitting an additional failure
> >> > message when NULL is returned.
> >> > …”
> >>
> >> Are you sure that's actually true?
> >>
> >> A quick look around in slub.c leads me to:
> >>
> >> slab_out_of_memory(struct kmem_cache *s, gfp_t gfpflags, int nid)
> >> {
> >> #ifdef CONFIG_SLUB_DEBUG
> >
> > You first have to enable EXPERT mode before you can disable SLUB_DEBUG.
>
> I see ~175 defconfigs with CONFIG_EXPERT=y, so that's not really a high
> bar unfortunately.
>
> And there's 38 defconfigs with SLUB_DEBUG=n.
>
> So for kernels built with those defconfigs that documentation is plain
> wrong and misleading.
>
> And then there's SLOB which doesn't dump stack anywhere AFAICS.
>
> In fact slab_out_of_memory() doesn't emit a stack dump either, it just
> prints a bunch of slab related info!
>
> > So that hopefully means you *really* want to save memory. It doesn't
> > make sense to add a bunch of memory wasting printks when the users want
> > to go to extra lengths to conserve memory.
>
> I agree that in many cases those printks are just a waste of space in
> the source and the binary and should be removed.
>
> But I dislike being told "these generic allocation functions all emit a
> stack dump" only to find out that actually they don't, they print some
> other debug info, and depending on config settings they actually don't
> print _anything_.
Wait... It *does* print a stack trace. We must but looking at the
wrong function. Huh... The stack trace comes from warn_alloc(). What
happen is this:
mm/slub.c
2673
2674 freelist = new_slab_objects(s, gfpflags, node, );
2675
2676 if (unlikely(!freelist)) {
2677 slab_out_of_memory(s, gfpflags, node);
2678 return NULL;
2679 }
2680
The new_slab_objects() will call allocate_slab() which calls
__alloc_pages_slowpath() which calls warn_alloc() on failure.
There are some error paths from alloc_pages() which look like they
could return without the stack dump, but those are impossible paths from
kmalloc or error injection.
regards,
dan carpenter
Re: [PATCH] powerpc/nvram: Replace kmalloc with kzalloc in the error message
On Tue, Jun 02, 2020 at 09:23:57PM +1000, Michael Ellerman wrote:
> Markus Elfring writes:
> >>>> Please just remove the message instead, it's a tiny allocation that's
> >>>> unlikely to ever fail, and the caller will print an error anyway.
> >>>
> >>> How do you think about to take another look at a previous update
> >>> suggestion
> >>> like the following?
> >>>
> >>> powerpc/nvram: Delete three error messages for a failed memory allocation
> >>> https://patchwork.ozlabs.org/project/linuxppc-dev/patch/00845261-8528-d011-d3b8-e9355a231...@users.sourceforge.net/
> >>> https://lore.kernel.org/linuxppc-dev/00845261-8528-d011-d3b8-e9355a231...@users.sourceforge.net/
> >>> https://lore.kernel.org/patchwork/patch/752720/
> >>> https://lkml.org/lkml/2017/1/19/537
> >>
> >> That deleted the messages from nvram_scan_partitions(), but neither of
> >> the callers of nvram_scan_paritions() check its return value or print
> >> anything if it fails. So removing those messages would make those
> >> failures silent which is not what we want.
> >
> > * How do you think about information like the following?
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=f359287765c04711ff54fbd11645271d8e5ff763#n883
> > “…
> > These generic allocation functions all emit a stack dump on failure when
> > used
> > without __GFP_NOWARN so there is no use in emitting an additional failure
> > message when NULL is returned.
> > …”
>
> Are you sure that's actually true?
>
> A quick look around in slub.c leads me to:
>
> slab_out_of_memory(struct kmem_cache *s, gfp_t gfpflags, int nid)
> {
> #ifdef CONFIG_SLUB_DEBUG
You first have to enable EXPERT mode before you can disable SLUB_DEBUG.
So that hopefully means you *really* want to save memory. It doesn't
make sense to add a bunch of memory wasting printks when the users want
to go to extra lengths to conserve memory.
regards,
dan carpenter
Re: [PATCH] usb: gadget: fsl: Fix a wrong judgment in fsl_udc_probe()
On Fri, Apr 10, 2020 at 04:05:06PM +0800, Tang Bin wrote: > > > > > > > Thus it must be fixed. > > Wording alternative: > >Thus adjust the error detection and corresponding exception handling. > > Got it. Wow... No, don't listen to Markus when it comes to writing commit messages. You couldn't find worse advice anywhere. :P regards, dan carpenter
Re: [PATCH][next] soc: fsl: dpio: fix dereference of pointer p before null check
On Fri, Feb 21, 2020 at 11:11:43PM +, Colin King wrote: > --- > drivers/soc/fsl/dpio/qbman-portal.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/soc/fsl/dpio/qbman-portal.c > b/drivers/soc/fsl/dpio/qbman-portal.c > index 740ee0d19582..d1f49caa5b13 100644 > --- a/drivers/soc/fsl/dpio/qbman-portal.c > +++ b/drivers/soc/fsl/dpio/qbman-portal.c > @@ -249,10 +249,11 @@ struct qbman_swp *qbman_swp_init(const struct > qbman_swp_desc *d) > u32 mask_size; > u32 eqcr_pi; > > - spin_lock_init(>access_spinlock); > - > if (!p) > return NULL; > + > + spin_lock_init(>access_spinlock); Allocations in the declaration blog are not super common in the kernel, but they're more bug prone. Generally, it's not beautiful to call a function which can fail in the allocation block. regards, dan carpenter
Re: [PATCH 0/4] use mmgrab
On Sun, Dec 29, 2019 at 04:42:54PM +0100, Julia Lawall wrote:
> Mmgrab was introduced in commit f1f1007644ff ("mm: add new mmgrab()
> helper") and most of the kernel was updated to use it. Update a few
> remaining files.
I wonder if there is an automatic way to generate these kind of
Coccinelle scripts which use inlines instead of open coding. Like maybe
make a list of one line functions, and then auto generate a recipe. Or
the mmgrab() function could have multiple lines if the first few were
just sanity checks for NULL or something...
regards,
dan carpenter
[PATCH] ASoC: fsl_mqs: Fix error handling in probe
There are several problems in the error handling in fsl_mqs_probe().
1) "ret" isn't initialized on some paths. GCC has a feature which
warns about uninitialized variables but the code initializes "ret"
to zero at the start of the function so the checking is turned off.
2) "gpr_np" is a pointer so initializing it to zero is confusing and
generates a Sparse warning.
3) of_parse_phandle() doesn't return error pointers on error, it returns
NULL.
4) If devm_snd_soc_register_component() fails then the function should
free the "gpr_np".
Fixes: 9e28f6532c61 ("ASoC: fsl_mqs: Add MQS component driver")
Signed-off-by: Dan Carpenter
---
sound/soc/fsl/fsl_mqs.c | 27 +++
1 file changed, 15 insertions(+), 12 deletions(-)
diff --git a/sound/soc/fsl/fsl_mqs.c b/sound/soc/fsl/fsl_mqs.c
index c1619a553514..f5f2f659bcbf 100644
--- a/sound/soc/fsl/fsl_mqs.c
+++ b/sound/soc/fsl/fsl_mqs.c
@@ -179,10 +179,10 @@ static const struct regmap_config fsl_mqs_regmap_config =
{
static int fsl_mqs_probe(struct platform_device *pdev)
{
struct device_node *np = pdev->dev.of_node;
- struct device_node *gpr_np = 0;
+ struct device_node *gpr_np = NULL;
struct fsl_mqs *mqs_priv;
void __iomem *regs;
- int ret = 0;
+ int ret;
mqs_priv = devm_kzalloc(>dev, sizeof(*mqs_priv), GFP_KERNEL);
if (!mqs_priv)
@@ -199,17 +199,16 @@ static int fsl_mqs_probe(struct platform_device *pdev)
if (mqs_priv->use_gpr) {
gpr_np = of_parse_phandle(np, "gpr", 0);
- if (IS_ERR(gpr_np)) {
+ if (!gpr_np) {
dev_err(>dev, "failed to get gpr node by
phandle\n");
- ret = PTR_ERR(gpr_np);
- goto out;
+ return -EINVAL;
}
mqs_priv->regmap = syscon_node_to_regmap(gpr_np);
if (IS_ERR(mqs_priv->regmap)) {
dev_err(>dev, "failed to get gpr regmap\n");
ret = PTR_ERR(mqs_priv->regmap);
- goto out;
+ goto err_free_gpr_np;
}
} else {
regs = devm_platform_ioremap_resource(pdev, 0);
@@ -230,7 +229,7 @@ static int fsl_mqs_probe(struct platform_device *pdev)
if (IS_ERR(mqs_priv->ipg)) {
dev_err(>dev, "failed to get the clock: %ld\n",
PTR_ERR(mqs_priv->ipg));
- goto out;
+ return PTR_ERR(mqs_priv->ipg);
}
}
@@ -238,17 +237,21 @@ static int fsl_mqs_probe(struct platform_device *pdev)
if (IS_ERR(mqs_priv->mclk)) {
dev_err(>dev, "failed to get the clock: %ld\n",
PTR_ERR(mqs_priv->mclk));
- goto out;
+ ret = PTR_ERR(mqs_priv->mclk);
+ goto err_free_gpr_np;
}
dev_set_drvdata(>dev, mqs_priv);
pm_runtime_enable(>dev);
- return devm_snd_soc_register_component(>dev, _codec_fsl_mqs,
+ ret = devm_snd_soc_register_component(>dev, _codec_fsl_mqs,
_mqs_dai, 1);
-out:
- if (!IS_ERR(gpr_np))
- of_node_put(gpr_np);
+ if (ret)
+ goto err_free_gpr_np;
+ return 0;
+
+err_free_gpr_np:
+ of_node_put(gpr_np);
return ret;
}
--
2.20.1
[bug report] hwmon: (ibmpowernv) Add attributes to enable/disable sensor groups
Hello Shilpasri G Bhat,
The patch e0da99123f3c: "hwmon: (ibmpowernv) Add attributes to
enable/disable sensor groups" from Jul 24, 2018, leads to the
following static checker warning:
drivers/hwmon/ibmpowernv.c:353 init_sensor_group_data()
warn: missing error code here? 'sgrp()' failed. 'ret' = '0'
drivers/hwmon/ibmpowernv.c
334 static int init_sensor_group_data(struct platform_device *pdev,
335struct platform_data *pdata)
336 {
337 struct sensor_group_data *sgrp_data;
338 struct device_node *groups, *sgrp;
339 int count = 0, ret = 0;
340 enum sensors type;
341
342 groups = of_find_compatible_node(NULL, NULL,
"ibm,opal-sensor-group");
343 if (!groups)
344 return ret;
To me the intent would be more clear if we said "return 0;". I look at
that I think maybe it's a bug.
345
346 for_each_child_of_node(groups, sgrp) {
347 type = get_sensor_type(sgrp);
348 if (type != MAX_SENSOR_TYPE)
349 pdata->nr_sensor_groups++;
350 }
351
352 if (!pdata->nr_sensor_groups)
353 goto out;
And here? Is this still a success path?
354
355 sgrp_data = devm_kcalloc(>dev, pdata->nr_sensor_groups,
356 sizeof(*sgrp_data), GFP_KERNEL);
357 if (!sgrp_data) {
358 ret = -ENOMEM;
359 goto out;
360 }
361
362 for_each_child_of_node(groups, sgrp) {
363 u32 gid;
364
365 type = get_sensor_type(sgrp);
regards,
dan carpenter
[bug report] powerpc/iommu: Implement IOMMU pools to improve multiqueue adapter performance
[ Ancient code. The warning is correct but the bug seems harmless.
-- dan ]
Hello Anton Blanchard,
The patch b4c3a8729ae5: "powerpc/iommu: Implement IOMMU pools to
improve multiqueue adapter performance" from Jun 7, 2012, leads to
the following static checker warning:
arch/powerpc/kernel/iommu.c:377 get_pool()
warn: array off by one? '*tbl->pools + pool_nr'
arch/powerpc/kernel/iommu.c
364 static struct iommu_pool *get_pool(struct iommu_table *tbl,
365 unsigned long entry)
366 {
367 struct iommu_pool *p;
368 unsigned long largepool_start = tbl->large_pool.start;
369
370 /* The large pool is the last pool at the top of the table */
371 if (entry >= largepool_start) {
372 p = >large_pool;
373 } else {
374 unsigned int pool_nr = entry / tbl->poolsize;
375
376 BUG_ON(pool_nr > tbl->nr_pools);
^
This should be ">=". The tbl->nr_pools value is either 1 or
IOMMU_NR_POOLS and the tbl->pools[] array has IOMMU_NR_POOLS elements.
377 p = >pools[pool_nr];
378 }
379
380 return p;
381 }
regards,
dan carpenter
Re: [PATCH 09/16] mmc: sdhci-xenon: use new match_string() helper/macro
On Fri, May 10, 2019 at 09:13:26AM +, Ardelean, Alexandru wrote:
> On Wed, 2019-05-08 at 16:26 +0300, Alexandru Ardelean wrote:
> > On Wed, 2019-05-08 at 15:20 +0300, Dan Carpenter wrote:
> > >
> > >
> > > On Wed, May 08, 2019 at 02:28:35PM +0300, Alexandru Ardelean wrote:
> > > > -static const char * const phy_types[] = {
> > > > - "emmc 5.0 phy",
> > > > - "emmc 5.1 phy"
> > > > -};
> > > > -
> > > > enum xenon_phy_type_enum {
> > > > EMMC_5_0_PHY,
> > > > EMMC_5_1_PHY,
> > > > NR_PHY_TYPES
> > >
> > > There is no need for NR_PHY_TYPES now so you could remove that as well.
> > >
> >
> > I thought the same.
> > The only reason to keep NR_PHY_TYPES, is for potential future patches,
> > where it would be just 1 addition
> >
> > enum xenon_phy_type_enum {
> > EMMC_5_0_PHY,
> > EMMC_5_1_PHY,
> > + EMMC_5_2_PHY,
> > NR_PHY_TYPES
> > }
> >
> > Depending on style/preference of how to do enums (allow comma on last
> > enum
> > or not allow comma on last enum value), adding new enum values woudl be 2
> > additions + 1 deletion lines.
> >
> > enum xenon_phy_type_enum {
> > EMMC_5_0_PHY,
> > - EMMC_5_1_PHY
> > + EMM
> > C_5_1_PHY,
> > + EMMC_5_2_PHY
> > }
> >
> > Either way (leave NR_PHY_TYPES or remove NR_PHY_TYPES) is fine from my
> > side.
> >
>
> Preference on this ?
> If no objection [nobody insists] I would keep.
>
> I don't feel strongly about it [dropping NR_PHY_TYPES or not].
If you end up resending the series could you remove it, but if not then
it's not worth it.
regards,
dan carpenter
Re: [PATCH 09/16] mmc: sdhci-xenon: use new match_string() helper/macro
On Wed, May 08, 2019 at 02:28:35PM +0300, Alexandru Ardelean wrote:
> -static const char * const phy_types[] = {
> - "emmc 5.0 phy",
> - "emmc 5.1 phy"
> -};
> -
> enum xenon_phy_type_enum {
> EMMC_5_0_PHY,
> EMMC_5_1_PHY,
> NR_PHY_TYPES
There is no need for NR_PHY_TYPES now so you could remove that as well.
regards,
dan carpenter
Re: [PATCH net-next 1/3] net: ethernet: support of_get_mac_address new ERR_PTR error
On Mon, May 06, 2019 at 11:58:35AM +0200, Petr Štetiar wrote: > There was NVMEM support added to of_get_mac_address, so it could now return > ERR_PTR encoded error values, so we need to adjust all current users of > of_get_mac_address to this new fact. We need a Fixes tag so we can look at the commit which adds NVMEM support. It's not clear to me that anyone ever applied that patch. If not then who hoo! Let's not apply it. But if it has been committed then it has a git hash. regards, dan carpenter
Re: [PATCH] soc: fsl: qe: gpio: Fix an error code in qe_pin_request()
On Fri, May 03, 2019 at 01:45:07PM -0500, Li Yang wrote: > On Fri, May 3, 2019 at 8:19 AM Dan Carpenter wrote: > > > > There was a missing error code in this path. It meant that we returned > > ERR_PTR(0) which is NULL and would result in a NULL dereference in the > > caller. > > Thanks Dan. An early version of this patch has been included in a > pending pull request to arm-soc. > https://lkml.org/lkml/2019/5/1/506 Oops. Sorry, I switched computers and forgot to copy my old outbox over. :( regards, dan carpenter
[PATCH] soc: fsl: qe: gpio: Fix an error code in qe_pin_request()
There was a missing error code in this path. It meant that we returned
ERR_PTR(0) which is NULL and would result in a NULL dereference in the
caller.
Fixes: 1a2d397a6eb5 ("gpio/powerpc: Eliminate duplication of
of_get_named_gpio_flags()")
Signed-off-by: Dan Carpenter
---
drivers/soc/fsl/qe/gpio.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/soc/fsl/qe/gpio.c b/drivers/soc/fsl/qe/gpio.c
index 819bed0f5667..51b3a47b5a55 100644
--- a/drivers/soc/fsl/qe/gpio.c
+++ b/drivers/soc/fsl/qe/gpio.c
@@ -179,8 +179,10 @@ struct qe_pin *qe_pin_request(struct device_node *np, int
index)
if (err < 0)
goto err0;
gc = gpio_to_chip(err);
- if (WARN_ON(!gc))
+ if (WARN_ON(!gc)) {
+ err = -ENODEV;
goto err0;
+ }
if (!of_device_is_compatible(gc->of_node, "fsl,mpc8323-qe-pario-bank"))
{
pr_debug("%s: tried to get a non-qe pin\n", __func__);
--
2.18.0
Re: [5/7] cpufreq/pasemi: Checking implementation of pas_cpufreq_cpu_init()
On Wed, Apr 03, 2019 at 04:23:54PM +0200, Markus Elfring wrote: > > @@ -146,6 +146,7 @@ static int pas_cpufreq_cpu_init(struct cpufreq_policy > > *policy) > > > > cpu = of_get_cpu_node(policy->cpu, NULL); > > > > + of_node_put(cpu); > > if (!cpu) > > goto out; > > Can the statement “return -ENODEV” be nicer as exception handling > in the if branch of this source code place? > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/cpufreq/pasemi-cpufreq.c?id=bf97b82f37c6d90e16de001d0659644c57fa490d#n137 > Why am I only receiving only one side of this conversation? I don't know why you're responding to... It's not required to fix/change unrelated style choices. If people want, they can just focus on their own thing. regards, dan carpenter
[PATCH] soc/fsl/qe: Fix an error code in qe_pin_request()
We forgot to set "err" on this error path.
Fixes: 1a2d397a6eb5 ("gpio/powerpc: Eliminate duplication of
of_get_named_gpio_flags()")
Signed-off-by: Dan Carpenter
---
drivers/soc/fsl/qe/gpio.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/soc/fsl/qe/gpio.c b/drivers/soc/fsl/qe/gpio.c
index 819bed0f5667..51b3a47b5a55 100644
--- a/drivers/soc/fsl/qe/gpio.c
+++ b/drivers/soc/fsl/qe/gpio.c
@@ -179,8 +179,10 @@ struct qe_pin *qe_pin_request(struct device_node *np, int
index)
if (err < 0)
goto err0;
gc = gpio_to_chip(err);
- if (WARN_ON(!gc))
+ if (WARN_ON(!gc)) {
+ err = -ENODEV;
goto err0;
+ }
if (!of_device_is_compatible(gc->of_node, "fsl,mpc8323-qe-pario-bank"))
{
pr_debug("%s: tried to get a non-qe pin\n", __func__);
--
2.17.1
[bug report] powerpc/mm: dump block address translation on book3s/32
Hello Christophe Leroy,
The patch 7c91efce1608: "powerpc/mm: dump block address translation
on book3s/32" from Dec 3, 2018, leads to the following static checker
warning:
arch/powerpc/mm/dump_bats.c:20 pp_601()
warn: both sides of ternary the same: '"RWX"'
arch/powerpc/mm/dump_bats.c
13 static char *pp_601(int k, int pp)
14 {
15 if (pp == 0)
16 return k ? "NA" : "RWX";
17 if (pp == 1)
18 return k ? "ROX" : "RWX";
19 if (pp == 2)
--> 20 return k ? "RWX" : "RWX";
^^^ ^^^
21 return k ? "ROX" : "ROX";
^^^ ^^^
Was something else intended here? Or we could make it simpler:
if (pp == 2)
return "RWX";
return "ROX";
22 }
regards,
dan carpenter
[PATCH] soc: fsl: dpio: Use after free in dpaa2_dpio_remove()
The dpaa2_io_down(priv->io) call frees "priv->io" so I've shifted the
code around a little bit to avoid the use after free.
Fixes: 991e873223e9 ("soc: fsl: dpio: use a cpumask to identify which cpus are
unused")
Signed-off-by: Dan Carpenter
---
drivers/soc/fsl/dpio/dpio-driver.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/soc/fsl/dpio/dpio-driver.c
b/drivers/soc/fsl/dpio/dpio-driver.c
index 2d4af32a0dec..a28799b62d53 100644
--- a/drivers/soc/fsl/dpio/dpio-driver.c
+++ b/drivers/soc/fsl/dpio/dpio-driver.c
@@ -220,12 +220,12 @@ static int dpaa2_dpio_remove(struct fsl_mc_device
*dpio_dev)
dev = _dev->dev;
priv = dev_get_drvdata(dev);
+ cpu = dpaa2_io_get_cpu(priv->io);
dpaa2_io_down(priv->io);
dpio_teardown_irqs(dpio_dev);
- cpu = dpaa2_io_get_cpu(priv->io);
cpumask_set_cpu(cpu, cpus_unused_mask);
err = dpio_open(dpio_dev->mc_io, 0, dpio_dev->obj_desc.id,
--
2.17.1
Re: [PATCH] powerpc/powernv/npu: Allocate enough memory in pnv_try_setup_npu_table_group()
On Sat, Jan 12, 2019 at 01:34:42AM -0600, Segher Boessenkool wrote:
> On Sat, Jan 12, 2019 at 08:44:26AM +0300, Dan Carpenter wrote:
> > On Sat, Jan 12, 2019 at 11:30:35AM +1100, Balbir Singh wrote:
> > > On Wed, Jan 09, 2019 at 01:23:29PM +0300, Dan Carpenter wrote:
> > > > There is a typo so we accidentally allocate enough memory for a pointer
> > > > when we wanted to allocate enough for a struct.
> > > >
> > > > Fixes: 0bd971676e68 ("powerpc/powernv/npu: Add compound IOMMU groups")
> > > > Signed-off-by: Dan Carpenter
> > > > ---
> > > > arch/powerpc/platforms/powernv/npu-dma.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/arch/powerpc/platforms/powernv/npu-dma.c
> > > > b/arch/powerpc/platforms/powernv/npu-dma.c
> > > > index d7f742ed48ba..3f58c7dbd581 100644
> > > > --- a/arch/powerpc/platforms/powernv/npu-dma.c
> > > > +++ b/arch/powerpc/platforms/powernv/npu-dma.c
> > > > @@ -564,7 +564,7 @@ struct iommu_table_group
> > > > *pnv_try_setup_npu_table_group(struct pnv_ioda_pe *pe)
> > > > }
> > > > } else {
> > > > /* Create a group for 1 GPU and attached NPUs for
> > > > POWER8 */
> > > > - pe->npucomp = kzalloc(sizeof(pe->npucomp), GFP_KERNEL);
> > > > + pe->npucomp = kzalloc(sizeof(*pe->npucomp), GFP_KERNEL);
> > >
> > > To avoid these in the future, I wonder if instead of sizeof(pe->npucomp),
> > > we insist on
> > > sizeof structure
> > >
> > > pe->npucomp = kzalloc(sizeof(struct npucomp), GFP_KERNEL);
> > >
> >
> > The latest kernel fashion is sizeof(*ptr). It can go wrong either way.
> > I don't have strong feelings about it. These sorts of bugs don't last
> > long because they're caught in testing or with static analysis.
>
> And it is easy to see someone forgot the * in "sizeof *ptr", and with
> experience it will just automatically look wrong if it is forgotten; but
> it isn't obvious at all if the wrong struct is used, which cannot happen
> with the *ptr form, but happens frequently with the "sizeof(struct x)"
> form.
It doesn't happen very frequently. I look for this with Smatch. The
results I see are when the first few members of a struct are a header
and the actual size can vary.
hdr = alloc(sizeof(struct larger_struct));
regards,
dan carpenter
Re: [PATCH] powerpc/powernv/npu: Allocate enough memory in pnv_try_setup_npu_table_group()
On Sat, Jan 12, 2019 at 11:30:35AM +1100, Balbir Singh wrote:
> On Wed, Jan 09, 2019 at 01:23:29PM +0300, Dan Carpenter wrote:
> > There is a typo so we accidentally allocate enough memory for a pointer
> > when we wanted to allocate enough for a struct.
> >
> > Fixes: 0bd971676e68 ("powerpc/powernv/npu: Add compound IOMMU groups")
> > Signed-off-by: Dan Carpenter
> > ---
> > arch/powerpc/platforms/powernv/npu-dma.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/powerpc/platforms/powernv/npu-dma.c
> > b/arch/powerpc/platforms/powernv/npu-dma.c
> > index d7f742ed48ba..3f58c7dbd581 100644
> > --- a/arch/powerpc/platforms/powernv/npu-dma.c
> > +++ b/arch/powerpc/platforms/powernv/npu-dma.c
> > @@ -564,7 +564,7 @@ struct iommu_table_group
> > *pnv_try_setup_npu_table_group(struct pnv_ioda_pe *pe)
> > }
> > } else {
> > /* Create a group for 1 GPU and attached NPUs for POWER8 */
> > - pe->npucomp = kzalloc(sizeof(pe->npucomp), GFP_KERNEL);
> > + pe->npucomp = kzalloc(sizeof(*pe->npucomp), GFP_KERNEL);
>
> To avoid these in the future, I wonder if instead of sizeof(pe->npucomp), we
> insist on
> sizeof structure
>
> pe->npucomp = kzalloc(sizeof(struct npucomp), GFP_KERNEL);
>
The latest kernel fashion is sizeof(*ptr). It can go wrong either way.
I don't have strong feelings about it. These sorts of bugs don't last
long because they're caught in testing or with static analysis.
regards,
dan carpenter
[PATCH] powerpc/powernv/npu: Allocate enough memory in pnv_try_setup_npu_table_group()
There is a typo so we accidentally allocate enough memory for a pointer
when we wanted to allocate enough for a struct.
Fixes: 0bd971676e68 ("powerpc/powernv/npu: Add compound IOMMU groups")
Signed-off-by: Dan Carpenter
---
arch/powerpc/platforms/powernv/npu-dma.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/platforms/powernv/npu-dma.c
b/arch/powerpc/platforms/powernv/npu-dma.c
index d7f742ed48ba..3f58c7dbd581 100644
--- a/arch/powerpc/platforms/powernv/npu-dma.c
+++ b/arch/powerpc/platforms/powernv/npu-dma.c
@@ -564,7 +564,7 @@ struct iommu_table_group
*pnv_try_setup_npu_table_group(struct pnv_ioda_pe *pe)
}
} else {
/* Create a group for 1 GPU and attached NPUs for POWER8 */
- pe->npucomp = kzalloc(sizeof(pe->npucomp), GFP_KERNEL);
+ pe->npucomp = kzalloc(sizeof(*pe->npucomp), GFP_KERNEL);
table_group = >npucomp->table_group;
table_group->ops = _npu_peers_ops;
iommu_register_group(table_group, hose->global_number,
--
2.17.1
Re: [PATCH] powerpc/ipic: Fix a bounds check in ipic_set_priority()
On Thu, Dec 06, 2018 at 09:12:12AM +0100, Julia Lawall wrote:
>
>
> On Thu, 6 Dec 2018, Christophe LEROY wrote:
>
> >
> >
> > Le 05/12/2018 à 04:26, Michael Ellerman a écrit :
> > > Hi Dan,
> > >
> > > Thanks for the patch.
> > >
> > > Dan Carpenter writes:
> > > > The ipic_info[] array only has 95 elements so I have made the bounds
> > > > check smaller to prevent a read overflow. It was Smatch that found
> > > > this issue:
> > > >
> > > > arch/powerpc/sysdev/ipic.c:784 ipic_set_priority()
> > > > error: buffer overflow 'ipic_info' 95 <= 127
> > > >
> > > > Signed-off-by: Dan Carpenter
> > > > ---
> > > > I wasn't able to find any callers of this code. Maybe we removed the
> > > > last one in commit b9f0f1bb2bca ("[POWERPC] Adapt ipic driver to new
> > > > host_ops interface, add set_irq_type to set IRQ sense"). So perhaps we
> > > > should just remove it. I'm not really comfortable doing that myself,
> > > > because I don't know the code well enough and can't build test
> > > > it properly.
> > >
> > > Hah wow, last usage removed in 2006!
> > >
> > > I don't see any mention of it since then, so I'll remove it. If it
> > > breaks something we can put it back.
> > >
> > > Can smatch help us find things like this that are defined non-static but
> > > never used?
> > >
> >
> > I think we have to do that carrefully. Some of those functions might be used
> > by out-of-tree boards.
> >
> > I'm thinking especially at ipic_get_mcp_status() and ipic_set_mcp_status().
> > They are used in my 832x boards's machine check handler to know when a
> > machine
> > check is a timeout from the 832x watchdog.
>
> The message I have gotten in the past is that the Linux kernel doesn't
> support code that is not used in the Linux kernel. However, if I were to
> do this, I would send the code to the individual maintainers, who
> presumably would know what is actually needed and what is not.
>
> Perhaps a good sanity check would be if the code has been used in the
> past. If there was a use in the past that has been removed, then perhaps
> it is more likely that the function was intended for internal kernel use
> rather than the case that you are describing.
Yeah. That's a good idea. I've been encouraging people who remove
"written to but not used" variables to say in the commit message
"variable foo has not been used since commit 123412341243 ("blah blah")."
It helps me review the patch as well.
regards,
dan carpenter
Re: [PATCH] powerpc/ipic: Fix a bounds check in ipic_set_priority()
On Wed, Dec 05, 2018 at 02:26:47PM +1100, Michael Ellerman wrote: > Can smatch help us find things like this that are defined non-static but > never used? > It's too tricky because it depends on the .config as well. regards, dan carpenter
[PATCH] powerpc/ipic: Fix a bounds check in ipic_set_priority()
The ipic_info[] array only has 95 elements so I have made the bounds
check smaller to prevent a read overflow. It was Smatch that found
this issue:
arch/powerpc/sysdev/ipic.c:784 ipic_set_priority()
error: buffer overflow 'ipic_info' 95 <= 127
Signed-off-by: Dan Carpenter
---
I wasn't able to find any callers of this code. Maybe we removed the
last one in commit b9f0f1bb2bca ("[POWERPC] Adapt ipic driver to new
host_ops interface, add set_irq_type to set IRQ sense"). So perhaps we
should just remove it. I'm not really comfortable doing that myself,
because I don't know the code well enough and can't build test
it properly.
arch/powerpc/sysdev/ipic.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/sysdev/ipic.c b/arch/powerpc/sysdev/ipic.c
index 6300123ce965..9d70d0687cd9 100644
--- a/arch/powerpc/sysdev/ipic.c
+++ b/arch/powerpc/sysdev/ipic.c
@@ -779,7 +779,7 @@ int ipic_set_priority(unsigned int virq, unsigned int
priority)
if (priority > 7)
return -EINVAL;
- if (src > 127)
+ if (src >= ARRAY_SIZE(ipic_info))
return -EINVAL;
if (ipic_info[src].prio == 0)
return -EINVAL;
--
2.11.0
Re: [PATCH V5 00/10] x86/KVM/Hyper-v: Add HV ept tlb range flush hypercall support in KVM
On Tue, Nov 27, 2018 at 07:59:22PM +0800, Tianyu Lan wrote: > Gentile Ping... > > On Thu, Nov 8, 2018 at 10:43 PM wrote: > > > > From: Lan Tianyu > > > > Sorry. Some patches was blocked and I try to resend via another account. The patches were still blocked? They didn't show up on driver-devel. regards, dan carpenter
[bug report] powerpc/perf: Add nest IMC PMU support
Hello Anju T Sudhakar,
The patch 885dcd709ba9: "powerpc/perf: Add nest IMC PMU support" from
Jul 19, 2017, leads to the following static checker warning:
arch/powerpc/perf/imc-pmu.c:506 nest_imc_event_init()
warn: 'pcni' can't be NULL.
arch/powerpc/perf/imc-pmu.c
485 if (event->cpu < 0)
486 return -EINVAL;
487
488 pmu = imc_event_to_pmu(event);
489
490 /* Sanity check for config (event offset) */
491 if ((config & IMC_EVENT_OFFSET_MASK) > pmu->counter_mem_size)
492 return -EINVAL;
493
494 /*
495 * Nest HW counter memory resides in a per-chip reserve-memory
(HOMER).
496 * Get the base memory addresss for this cpu.
497 */
498 chip_id = cpu_to_chip_id(event->cpu);
499 pcni = pmu->mem_info;
500 do {
501 if (pcni->id == chip_id) {
502 flag = true;
503 break;
504 }
505 pcni++;
^^
506 } while (pcni);
This will loop until we crash. I'm not sure what was intended.
507
508 if (!flag)
509 return -ENODEV;
510
regards,
dan carpenter
Re: [PATCH] powerpc: signedness bug in update_flash_db()
On Mon, Oct 01, 2018 at 10:02:54PM +0300, Dan Carpenter wrote:
> On Mon, Oct 01, 2018 at 08:22:01PM +0200, christophe leroy wrote:
> >
> >
> > Le 01/10/2018 à 18:44, Dan Carpenter a écrit :
> > > The "count < sizeof(struct os_area_db)" comparison is type promoted to
> > > size_t so negative values of "count" are treated as very high values and
> > > we accidentally return success instead of a negative error code.
> > >
> > > This doesn't really change runtime much but it fixes a static checker
> > > warning.
> > >
> > > Signed-off-by: Dan Carpenter
> > >
> > > diff --git a/arch/powerpc/platforms/ps3/os-area.c
> > > b/arch/powerpc/platforms/ps3/os-area.c
> > > index cdbfc5cfd6f3..f5387ad82279 100644
> > > --- a/arch/powerpc/platforms/ps3/os-area.c
> > > +++ b/arch/powerpc/platforms/ps3/os-area.c
> > > @@ -664,7 +664,7 @@ static int update_flash_db(void)
> > > db_set_64(db, _area_db_id_rtc_diff, saved_params.rtc_diff);
> > > count = os_area_flash_write(db, sizeof(struct os_area_db), pos);
> > > - if (count < sizeof(struct os_area_db)) {
> > > + if (count < 0 || count < sizeof(struct os_area_db)) {
> >
> > Why not simply add a cast ? :
> >
> > if (count < (ssize_t)sizeof(struct os_area_db)) {
> >
>
> There are so many ways to solve these and no accounting for taste. Do
> you need me to resend or can you redo it yourself?
>
Btw, I just went on vacation, and I'm not going to be back until next
week.
regards,
dan carpenter
Re: [PATCH] powerpc: signedness bug in update_flash_db()
On Mon, Oct 01, 2018 at 08:22:01PM +0200, christophe leroy wrote:
>
>
> Le 01/10/2018 à 18:44, Dan Carpenter a écrit :
> > The "count < sizeof(struct os_area_db)" comparison is type promoted to
> > size_t so negative values of "count" are treated as very high values and
> > we accidentally return success instead of a negative error code.
> >
> > This doesn't really change runtime much but it fixes a static checker
> > warning.
> >
> > Signed-off-by: Dan Carpenter
> >
> > diff --git a/arch/powerpc/platforms/ps3/os-area.c
> > b/arch/powerpc/platforms/ps3/os-area.c
> > index cdbfc5cfd6f3..f5387ad82279 100644
> > --- a/arch/powerpc/platforms/ps3/os-area.c
> > +++ b/arch/powerpc/platforms/ps3/os-area.c
> > @@ -664,7 +664,7 @@ static int update_flash_db(void)
> > db_set_64(db, _area_db_id_rtc_diff, saved_params.rtc_diff);
> > count = os_area_flash_write(db, sizeof(struct os_area_db), pos);
> > - if (count < sizeof(struct os_area_db)) {
> > + if (count < 0 || count < sizeof(struct os_area_db)) {
>
> Why not simply add a cast ? :
>
> if (count < (ssize_t)sizeof(struct os_area_db)) {
>
There are so many ways to solve these and no accounting for taste. Do
you need me to resend or can you redo it yourself?
regards,
dan carpenter
[PATCH] powerpc: signedness bug in update_flash_db()
The "count < sizeof(struct os_area_db)" comparison is type promoted to
size_t so negative values of "count" are treated as very high values and
we accidentally return success instead of a negative error code.
This doesn't really change runtime much but it fixes a static checker
warning.
Signed-off-by: Dan Carpenter
diff --git a/arch/powerpc/platforms/ps3/os-area.c
b/arch/powerpc/platforms/ps3/os-area.c
index cdbfc5cfd6f3..f5387ad82279 100644
--- a/arch/powerpc/platforms/ps3/os-area.c
+++ b/arch/powerpc/platforms/ps3/os-area.c
@@ -664,7 +664,7 @@ static int update_flash_db(void)
db_set_64(db, _area_db_id_rtc_diff, saved_params.rtc_diff);
count = os_area_flash_write(db, sizeof(struct os_area_db), pos);
- if (count < sizeof(struct os_area_db)) {
+ if (count < 0 || count < sizeof(struct os_area_db)) {
pr_debug("%s: os_area_flash_write failed %zd\n", __func__,
count);
error = count < 0 ? count : -EIO;
Re: [PATCH] fsl/qe: ucc: copy and paste bug in ucc_get_tdm_sync_shift()
On Mon, Jun 04, 2018 at 02:06:45PM +0200, Mathieu Malaterre wrote: > Where did the original go ? > > https://patchwork.ozlabs.org/patch/868158/ > > Btw, we still haven't pushed a patch for this bug. regards, dan carpenter
[PATCH] powerpc: fix size calculation using resource_size()
The problem is the the calculation should be "end - start + 1" but the
plus one is missing in this calculation.
Fixes: 8626816e905e ("powerpc: add support for MPIC message register API")
Signed-off-by: Dan Carpenter
---
Static analysis. Not tested.
diff --git a/arch/powerpc/sysdev/mpic_msgr.c b/arch/powerpc/sysdev/mpic_msgr.c
index eb69a5186243..280e964e1aa8 100644
--- a/arch/powerpc/sysdev/mpic_msgr.c
+++ b/arch/powerpc/sysdev/mpic_msgr.c
@@ -196,7 +196,7 @@ static int mpic_msgr_probe(struct platform_device *dev)
/* IO map the message register block. */
of_address_to_resource(np, 0, );
- msgr_block_addr = ioremap(rsrc.start, rsrc.end - rsrc.start);
+ msgr_block_addr = ioremap(rsrc.start, resource_size());
if (!msgr_block_addr) {
dev_err(>dev, "Failed to iomap MPIC message registers");
return -EFAULT;
[bug report] cxl: Prevent adapter reset if an active context exists
Hello Vaibhav Jain,
The patch 70b565bbdb91: "cxl: Prevent adapter reset if an active
context exists" from Oct 14, 2016, leads to the following static
checker warning:
drivers/misc/cxl/main.c:290 cxl_adapter_context_get()
warn: 'atomic_inc_unless_negative(>contexts_num)' is unsigned
drivers/misc/cxl/main.c
285 int cxl_adapter_context_get(struct cxl *adapter)
286 {
287 int rc;
288
289 rc = atomic_inc_unless_negative(>contexts_num);
290 return rc >= 0 ? 0 : -EBUSY;
atomic_inc_unless_negative() returns bool so it's always >= 0.
291 }
regards,
dan carpenter
Re: [PATCH 2/3] drivers/base: reorder consumer and its children behind suppliers
On Wed, Jun 27, 2018 at 10:34:54AM +0800, Pingfan Liu wrote: > > 1b2a1e63 Pingfan Liu 2018-06-25 243} > > 1b2a1e63 Pingfan Liu 2018-06-25 244} > > 1b2a1e63 Pingfan Liu 2018-06-25 @245BUG_ON(!ret); > > > > If the list is empty then "ret" can be unitialized. We test a different > > list "dev->links.suppliers" to see if that's empty. I wrote a bunch of > > code to make Smatch try to understand about empty lists, but I don't > > think it works... > > > Yes, if list_empty, then the code can not touch ret. But ret is > useless in this scene. Does it matter? > I'm not sure I understand what you're asking? Of course, it matters? regards, dan carpenter
Re: [PATCH 2/3] drivers/base: reorder consumer and its children behind suppliers
[ There is a bug with kbuild where it's not showing the Smatch warnings
but I can probably guess... - dan ]
Hi Pingfan,
Thank you for the patch! Perhaps something to improve:
url:
https://github.com/0day-ci/linux/commits/Pingfan-Liu/drivers-base-bugfix-for-supplier-consumer-ordering-in-device_kset/20180625-132702
#
https://github.com/0day-ci/linux/commit/1b2a1e63898baf80e8e830991284e1534bc54766
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 1b2a1e63898baf80e8e830991284e1534bc54766
vim +/ret +245 drivers/base/core.c
1b2a1e63 Pingfan Liu 2018-06-25 216
1b2a1e63 Pingfan Liu 2018-06-25 217 /* When reodering, take care of the range
of (old_pos(dev), new_pos(dev)),
1b2a1e63 Pingfan Liu 2018-06-25 218 * there may be requirement to
recursively move item.
1b2a1e63 Pingfan Liu 2018-06-25 219 */
1b2a1e63 Pingfan Liu 2018-06-25 220 int device_reorder_consumer(struct device
*dev)
1b2a1e63 Pingfan Liu 2018-06-25 221 {
1b2a1e63 Pingfan Liu 2018-06-25 222struct list_head *iter, *left, *right;
1b2a1e63 Pingfan Liu 2018-06-25 223struct device *cur_dev;
1b2a1e63 Pingfan Liu 2018-06-25 224struct pos_info info;
1b2a1e63 Pingfan Liu 2018-06-25 225int ret, idx;
1b2a1e63 Pingfan Liu 2018-06-25 226
1b2a1e63 Pingfan Liu 2018-06-25 227idx = device_links_read_lock();
1b2a1e63 Pingfan Liu 2018-06-25 228if (list_empty(>links.suppliers)) {
1b2a1e63 Pingfan Liu 2018-06-25 229device_links_read_unlock(idx);
1b2a1e63 Pingfan Liu 2018-06-25 230return 0;
1b2a1e63 Pingfan Liu 2018-06-25 231}
1b2a1e63 Pingfan Liu 2018-06-25 232spin_lock(_kset->list_lock);
1b2a1e63 Pingfan Liu 2018-06-25 233list_for_each_prev(iter,
_kset->list) {
1b2a1e63 Pingfan Liu 2018-06-25 234cur_dev = list_entry(iter,
struct device, kobj.entry);
1b2a1e63 Pingfan Liu 2018-06-25 235ret = find_last_supplier(dev,
cur_dev);
1b2a1e63 Pingfan Liu 2018-06-25 236switch (ret) {
1b2a1e63 Pingfan Liu 2018-06-25 237case -1:
1b2a1e63 Pingfan Liu 2018-06-25 238goto unlock;
1b2a1e63 Pingfan Liu 2018-06-25 239case 1:
1b2a1e63 Pingfan Liu 2018-06-25 240break;
1b2a1e63 Pingfan Liu 2018-06-25 241case 0:
1b2a1e63 Pingfan Liu 2018-06-25 242continue;
The break breaks from the switch and the continue continues the loop so
they're equivalent. Perhaps you intended to break from the loop?
1b2a1e63 Pingfan Liu 2018-06-25 243}
1b2a1e63 Pingfan Liu 2018-06-25 244}
1b2a1e63 Pingfan Liu 2018-06-25 @245BUG_ON(!ret);
If the list is empty then "ret" can be unitialized. We test a different
list "dev->links.suppliers" to see if that's empty. I wrote a bunch of
code to make Smatch try to understand about empty lists, but I don't
think it works...
1b2a1e63 Pingfan Liu 2018-06-25 246
1b2a1e63 Pingfan Liu 2018-06-25 247/* record the affected open section */
1b2a1e63 Pingfan Liu 2018-06-25 248left = dev->kobj.entry.prev;
1b2a1e63 Pingfan Liu 2018-06-25 249right = iter;
1b2a1e63 Pingfan Liu 2018-06-25 250info.pos = list_entry(iter, struct
device, kobj.entry);
1b2a1e63 Pingfan Liu 2018-06-25 251info.tail = NULL;
1b2a1e63 Pingfan Liu 2018-06-25 252/* dry out the consumers in
(left,right) */
1b2a1e63 Pingfan Liu 2018-06-25 253__device_reorder_consumer(dev, left,
right, );
1b2a1e63 Pingfan Liu 2018-06-25 254
1b2a1e63 Pingfan Liu 2018-06-25 255 unlock:
1b2a1e63 Pingfan Liu 2018-06-25 256spin_unlock(_kset->list_lock);
1b2a1e63 Pingfan Liu 2018-06-25 257device_links_read_unlock(idx);
1b2a1e63 Pingfan Liu 2018-06-25 258return 0;
1b2a1e63 Pingfan Liu 2018-06-25 259 }
1b2a1e63 Pingfan Liu 2018-06-25 260
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Re: [PATCH] fsl/qe: ucc: copy and paste bug in ucc_get_tdm_sync_shift()
On Mon, Jun 04, 2018 at 10:25:14PM +0900, Julia Lawall wrote:
>
>
> On Mon, 4 Jun 2018, Dan Carpenter wrote:
>
> > There is a copy and paste bug so we accidentally use the RX_ shift when
> > we're in TX_ mode.
> >
> > Fixes: bb8b2062aff3 ("fsl/qe: setup clock source for TDM mode")
> > Signed-off-by: Dan Carpenter
> > ---
> > Static analysis work. Not tested. This affects the success path, so
> > we should probably test it.
>
> Maybe this is another one? I don't have time to look into it at the
> moment...
>
> drivers/net/ethernet/broadcom/bnx2x/bnx2x_link.c
>
> /* For strict priority entries defines the number of consecutive
>* slots for the highest priority.
>*/
> REG_WR(bp, (port) ? NIG_REG_P1_TX_ARB_NUM_STRICT_ARB_SLOTS :
> NIG_REG_P1_TX_ARB_NUM_STRICT_ARB_SLOTS, 0x100);
> /* Mapping between the CREDIT_WEIGHT registers and actual client
>* numbers
>*/
>
> I find some others that choose between constants, such as ... ? 0 : 0.
I feel like it should warn about all of those because people shouldn't
be submitting unfinished written code to the kernel. Coccinelle is a
lot better for this than Smatch is because it's pre-processor stuff.
regards,
dan carpenter
[PATCH] fsl/qe: ucc: copy and paste bug in ucc_get_tdm_sync_shift()
There is a copy and paste bug so we accidentally use the RX_ shift when
we're in TX_ mode.
Fixes: bb8b2062aff3 ("fsl/qe: setup clock source for TDM mode")
Signed-off-by: Dan Carpenter
---
Static analysis work. Not tested. This affects the success path, so
we should probably test it.
diff --git a/drivers/soc/fsl/qe/ucc.c b/drivers/soc/fsl/qe/ucc.c
index c646d8713861..681f7d4b7724 100644
--- a/drivers/soc/fsl/qe/ucc.c
+++ b/drivers/soc/fsl/qe/ucc.c
@@ -626,7 +626,7 @@ static u32 ucc_get_tdm_sync_shift(enum comm_dir mode, u32
tdm_num)
{
u32 shift;
- shift = (mode == COMM_DIR_RX) ? RX_SYNC_SHIFT_BASE : RX_SYNC_SHIFT_BASE;
+ shift = (mode == COMM_DIR_RX) ? RX_SYNC_SHIFT_BASE : TX_SYNC_SHIFT_BASE;
shift -= tdm_num * 2;
return shift;
[bug report] powerpc/4xx: Adding PCIe MSI support
[ This is really really ancient code. - dan ]
Hello Rupjyoti Sarmah,
The patch 3fb7933850fa: "powerpc/4xx: Adding PCIe MSI support" from
Mar 29, 2011, leads to the following static checker warning:
arch/powerpc/platforms/4xx/msi.c:100 ppc4xx_setup_msi_irqs()
warn: 'int_no >= 0' 'false' implies 'int_no < 0' is 'true'
arch/powerpc/platforms/4xx/msi.c
79 static int ppc4xx_setup_msi_irqs(struct pci_dev *dev, int nvec, int
type)
80 {
81 int int_no = -ENOMEM;
82 unsigned int virq;
83 struct msi_msg msg;
84 struct msi_desc *entry;
85 struct ppc4xx_msi *msi_data = _msi;
86
87 dev_dbg(>dev, "PCIE-MSI:%s called. vec %x type %d\n",
88 __func__, nvec, type);
89 if (type == PCI_CAP_ID_MSIX)
90 pr_debug("ppc4xx msi: MSI-X untested, trying
anyway.\n");
91
92 msi_data->msi_virqs = kmalloc((msi_irqs) * sizeof(int),
GFP_KERNEL);
93 if (!msi_data->msi_virqs)
94 return -ENOMEM;
95
96 for_each_pci_msi_entry(entry, dev) {
97 int_no = msi_bitmap_alloc_hwirqs(_data->bitmap, 1);
98 if (int_no >= 0)
^^^
99 break;
100 if (int_no < 0) {
^^
Smatch is saying that this check could be removed, which is true.
101 pr_debug("%s: fail allocating msi interrupt\n",
102 __func__);
103 }
104 virq = irq_of_parse_and_map(msi_data->msi_dev, int_no);
^^
It doesn't seem right to pass negative indexes to irq_of_parse_and_map().
It could result in a read before the start of the array in
of_irq_parse_oldworld(), I think.
105 if (!virq) {
106 dev_err(>dev, "%s: fail mapping irq\n",
__func__);
107 msi_bitmap_free_hwirqs(_data->bitmap,
int_no, 1);
108 return -ENOSPC;
109 }
110 dev_dbg(>dev, "%s: virq = %d\n", __func__, virq);
111
112 /* Setup msi address space */
113 msg.address_hi = msi_data->msi_addr_hi;
114 msg.address_lo = msi_data->msi_addr_lo;
115
116 irq_set_msi_desc(virq, entry);
117 msg.data = int_no;
118 pci_write_msi_msg(virq, );
119 }
120 return 0;
121 }
regards,
dan carpenter
[PATCH net-next] ibmvnic: Potential NULL dereference in clean_one_tx_pool()
There is an && vs || typo here, which potentially leads to a NULL
dereference.
Fixes: e9e1e97884b7 ("ibmvnic: Update TX pool cleaning routine")
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
diff --git a/drivers/net/ethernet/ibm/ibmvnic.c
b/drivers/net/ethernet/ibm/ibmvnic.c
index 5632c030811b..0389a7a52152 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -1135,7 +1135,7 @@ static void clean_one_tx_pool(struct ibmvnic_adapter
*adapter,
u64 tx_entries;
int i;
- if (!tx_pool && !tx_pool->tx_buff)
+ if (!tx_pool || !tx_pool->tx_buff)
return;
tx_entries = tx_pool->num_buffers;
Re: [PATCH] powerpc: Use common error handling code in setup_new_fdt()
On Fri, Mar 16, 2018 at 09:26:53PM +1100, Michael Ellerman wrote: > Dan Carpenter <dan.carpen...@oracle.com> writes: > > On Wed, Mar 14, 2018 at 06:22:07PM -0300, Thiago Jung Bauermann wrote: > >> SF Markus Elfring <elfr...@users.sourceforge.net> writes: > >> > From: Markus Elfring <elfr...@users.sourceforge.net> > >> > Date: Sun, 11 Mar 2018 09:03:42 +0100 > >> > > >> > Add a jump target so that a bit of exception handling can be better > >> > reused > >> > at the end of this function. > >> > > >> > This issue was detected by using the Coccinelle software. > >> > > >> > Signed-off-by: Markus Elfring <elfr...@users.sourceforge.net> > >> > --- > >> > arch/powerpc/kernel/machine_kexec_file_64.c | 28 > >> > > >> > 1 file changed, 12 insertions(+), 16 deletions(-) > >> > >> I liked it. Thanks! > >> > >> Reviewed-by: Thiago Jung Bauermann <bauer...@linux.vnet.ibm.com> > > > > You know that compilers already re-use string constants so this doesn't > > actually save memory? > > Sure, but it's still clearer to only have the string appear once in the > code. > To me the original was better. > > Also we should be preserving the error codes > > instead of always returning -EINVAL. > > The error codes come from libfdt code, so they don't necessarily make > sense in the kernel. eg. FDT_ERR_NOSPACE == 3 == ESRCH. > > Perhaps we should be trying harder to convert them, but that's a > criticism of the original code not this patch. Ah. You're right. I look at the patch in context, sorry. regards, dan carpenter
Re: [PATCH] powerpc: Use common error handling code in setup_new_fdt()
On Wed, Mar 14, 2018 at 06:22:07PM -0300, Thiago Jung Bauermann wrote: > > SF Markus Elfring <elfr...@users.sourceforge.net> writes: > > > From: Markus Elfring <elfr...@users.sourceforge.net> > > Date: Sun, 11 Mar 2018 09:03:42 +0100 > > > > Add a jump target so that a bit of exception handling can be better reused > > at the end of this function. > > > > This issue was detected by using the Coccinelle software. > > > > Signed-off-by: Markus Elfring <elfr...@users.sourceforge.net> > > --- > > arch/powerpc/kernel/machine_kexec_file_64.c | 28 > > > > 1 file changed, 12 insertions(+), 16 deletions(-) > > I liked it. Thanks! > > Reviewed-by: Thiago Jung Bauermann <bauer...@linux.vnet.ibm.com> > You know that compilers already re-use string constants so this doesn't actually save memory? Also we should be preserving the error codes instead of always returning -EINVAL. regards, dan carpenter
Re: [bug report] ocxl: Add AFU interrupt support
On Tue, Feb 13, 2018 at 08:29:26PM +0100, Frederic Barrat wrote: > Hi, > > Thanks for the report. I'll fix the first issue. The 2nd is already on its > way to upstream: > https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git/commit/?id=dedab7f0d3137441a97fe7cf9b9ca5 > > (though we still have a useless cast in there; will fix as well). > > May I ask what static checker you're using? > These are Smatch warnings. regards, dan carpenter
[bug report] ocxl: Add AFU interrupt support
Hello Frederic Barrat,
The patch aeddad1760ae: "ocxl: Add AFU interrupt support" from Jan
23, 2018, leads to the following static checker warning:
drivers/misc/ocxl/file.c:163 afu_ioctl()
warn: maybe return -EFAULT instead of the bytes remaining?
drivers/misc/ocxl/file.c
111 static long afu_ioctl(struct file *file, unsigned int cmd,
112 unsigned long args)
113 {
114 struct ocxl_context *ctx = file->private_data;
115 struct ocxl_ioctl_irq_fd irq_fd;
116 u64 irq_offset;
117 long rc;
118
119 pr_debug("%s for context %d, command %s\n", __func__,
ctx->pasid,
120 CMD_STR(cmd));
121
122 if (ctx->status == CLOSED)
123 return -EIO;
124
125 switch (cmd) {
126 case OCXL_IOCTL_ATTACH:
127 rc = afu_ioctl_attach(ctx,
128 (struct ocxl_ioctl_attach __user *)
args);
129 break;
130
131 case OCXL_IOCTL_IRQ_ALLOC:
132 rc = ocxl_afu_irq_alloc(ctx, _offset);
133 if (!rc) {
134 rc = copy_to_user((u64 __user *) args,
_offset,
135 sizeof(irq_offset));
136 if (rc)
^^
copy_to_user() returns the number of bytes remaining but we want to
return -EFAULT on error.
137 ocxl_afu_irq_free(ctx, irq_offset);
138 }
139 break;
140
drivers/misc/ocxl/file.c:320 afu_read()
warn: unsigned 'used' is never less than zero.
drivers/misc/ocxl/file.c
279 ssize_t rc;
280 size_t used = 0;
^^
This should be ssize_t
281 DEFINE_WAIT(event_wait);
282
283 memset(, 0, sizeof(header));
284
285 /* Require offset to be 0 */
286 if (*off != 0)
287 return -EINVAL;
288
289 if (count < (sizeof(struct ocxl_kernel_event_header) +
290 AFU_EVENT_BODY_MAX_SIZE))
291 return -EINVAL;
292
293 for (;;) {
294 prepare_to_wait(>events_wq, _wait,
295 TASK_INTERRUPTIBLE);
296
297 if (afu_events_pending(ctx))
298 break;
299
300 if (ctx->status == CLOSED)
301 break;
302
303 if (file->f_flags & O_NONBLOCK) {
304 finish_wait(>events_wq, _wait);
305 return -EAGAIN;
306 }
307
308 if (signal_pending(current)) {
309 finish_wait(>events_wq, _wait);
310 return -ERESTARTSYS;
311 }
312
313 schedule();
314 }
315
316 finish_wait(>events_wq, _wait);
317
318 if (has_xsl_error(ctx)) {
319 used = append_xsl_error(ctx, , buf +
sizeof(header));
320 if (used < 0)
Impossible.
321 return used;
322 }
323
324 if (!afu_events_pending(ctx))
325 header.flags |= OCXL_KERNEL_EVENT_FLAG_LAST;
326
327 if (copy_to_user(buf, , sizeof(header)))
328 return -EFAULT;
329
330 used += sizeof(header);
331
332 rc = (ssize_t) used;
^^
You could remove the cast.
333 return rc;
334 }
regards,
dan carpenter
Re: [bug report] powerpc/mm/radix: Add tlbflush routines
On Wed, Jan 31, 2018 at 08:58:50PM -0800, Michael Ellerman wrote:
> Dan Carpenter <dan.carpen...@oracle.com> writes:
>
> > Hello Aneesh Kumar K.V,
> >
> > The patch 1a472c9dba6b: "powerpc/mm/radix: Add tlbflush routines"
> > from Apr 29, 2016, leads to the following static checker warning:
> >
> > arch/powerpc/mm/tlb_nohash.c:218 __local_flush_tlb_page()
> > warn: always true condition '(pid != ~0) => (0-u32max != u64max)'
> >
> > arch/powerpc/mm/tlb_nohash.c
> >211 void __local_flush_tlb_page(struct mm_struct *mm, unsigned long
> > vmaddr,
> >212 int tsize, int ind)
> >213 {
> >214 unsigned int pid;
> >215
> >216 preempt_disable();
> >217 pid = mm ? mm->context.id : 0;
> >218 if (pid != MMU_NO_CONTEXT)
> > ^
> >219 _tlbil_va(vmaddr, pid, tsize, ind);
> >220 preempt_enable();
> >221 }
> >
> > I don't know very much about PowerPC. The static checker is guessing
> > which headers to pull in instead of relying on the build system so there
> > are a lot of false positives.
>
> O_o
>
> That's a bit nuts ... :)
>
Heh.
Thanks for looking into this.
regards,
dan carpenter
[bug report] powerpc/perf: Add nest IMC PMU support
Hello Anju T Sudhakar,
The patch 885dcd709ba9: "powerpc/perf: Add nest IMC PMU support" from
Jul 19, 2017, leads to the following static checker warning:
arch/powerpc/perf/imc-pmu.c:1393 init_imc_pmu()
warn: 'pmu_ptr' was already freed.
arch/powerpc/perf/imc-pmu.c
1317 int init_imc_pmu(struct device_node *parent, struct imc_pmu *pmu_ptr,
int pmu_idx)
1318 {
1319 int ret;
1320
1321 ret = imc_mem_init(pmu_ptr, parent, pmu_idx);
1322 if (ret) {
1323 imc_common_mem_free(pmu_ptr);
1324 return ret;
1325 }
Change this to:
if (ret)
goto err_free_mpu_ptr;
Or something instead of a direct return. That's more normal kernel
style.
1326
1327 switch (pmu_ptr->domain) {
1328 case IMC_DOMAIN_NEST:
1329 /*
1330 * Nest imc pmu need only one cpu per chip, we
initialize the
1331 * cpumask for the first nest imc pmu and use the same
for the
1332 * rest. To handle the cpuhotplug callback unregister,
we track
1333 * the number of nest pmus in "nest_pmus".
1334 */
1335 mutex_lock(_init_lock);
1336 if (nest_pmus == 0) {
1337 ret = init_nest_pmu_ref();
1338 if (ret) {
1339 mutex_unlock(_init_lock);
1340 goto err_free;
1341 }
1342 /* Register for cpu hotplug notification. */
1343 ret = nest_pmu_cpumask_init();
1344 if (ret) {
1345 mutex_unlock(_init_lock);
1346 kfree(nest_imc_refc);
1347 kfree(per_nest_pmu_arr);
1348 goto err_free;
1349 }
1350 }
1351 nest_pmus++;
1352 mutex_unlock(_init_lock);
1353 break;
1354 case IMC_DOMAIN_CORE:
1355 ret = core_imc_pmu_cpumask_init();
1356 if (ret) {
1357 cleanup_all_core_imc_memory();
1358 return ret;
These direct returns don't look correct...
1359 }
1360
1361 break;
1362 case IMC_DOMAIN_THREAD:
1363 ret = thread_imc_cpu_init();
1364 if (ret) {
1365 cleanup_all_thread_imc_memory();
1366 return ret;
1367 }
1368
1369 break;
1370 default:
1371 return -1; /* Unknown domain */
This one certainly looks like a memory leak. Plus -1 is -EPERM which is
probably not the correct error code.
1372 }
1373
1374 ret = update_events_in_group(parent, pmu_ptr);
1375 if (ret)
1376 goto err_free;
1377
1378 ret = update_pmu_ops(pmu_ptr);
1379 if (ret)
1380 goto err_free;
1381
1382 ret = perf_pmu_register(_ptr->pmu, pmu_ptr->pmu.name, -1);
1383 if (ret)
1384 goto err_free;
1385
1386 pr_info("%s performance monitor hardware support registered\n",
1387
pmu_ptr->pmu.name);
1388
1389 return 0;
1390
1391 err_free:
1392 imc_common_mem_free(pmu_ptr);
1393 imc_common_cpuhp_mem_free(pmu_ptr);
^^^
This is a use after free, it should be in the reverse order.
err_free_cpuhp:
imc_common_cpuhp_mem_free(pmu_ptr);
err_free_pmu_ptr:
imc_common_mem_free(pmu_ptr);
1394 return ret;
1395 }
regards,
dan carpenter
[bug report] fsl/qe: setup clock source for TDM mode
Hello Zhao Qiang,
The patch bb8b2062aff3: "fsl/qe: setup clock source for TDM mode"
from Jun 6, 2016, leads to the following static checker warning:
drivers/soc/fsl/qe/ucc.c:629 ucc_get_tdm_sync_shift()
warn: both sides of ternary the same: '30' RX_SYNC_SHIFT_BASE
RX_SYNC_SHIFT_BASE
drivers/soc/fsl/qe/ucc.c
625 static u32 ucc_get_tdm_sync_shift(enum comm_dir mode, u32 tdm_num)
626 {
627 u32 shift;
628
629 shift = (mode == COMM_DIR_RX) ? RX_SYNC_SHIFT_BASE :
RX_SYNC_SHIFT_BASE;
^^
Maybe this one should have been TX_?
630 shift -= tdm_num * 2;
631
632 return shift;
633 }
regards,
dan carpenter
[bug report] powerpc/mm/radix: Add tlbflush routines
Hello Aneesh Kumar K.V,
The patch 1a472c9dba6b: "powerpc/mm/radix: Add tlbflush routines"
from Apr 29, 2016, leads to the following static checker warning:
arch/powerpc/mm/tlb_nohash.c:218 __local_flush_tlb_page()
warn: always true condition '(pid != ~0) => (0-u32max != u64max)'
arch/powerpc/mm/tlb_nohash.c
211 void __local_flush_tlb_page(struct mm_struct *mm, unsigned long vmaddr,
212 int tsize, int ind)
213 {
214 unsigned int pid;
215
216 preempt_disable();
217 pid = mm ? mm->context.id : 0;
218 if (pid != MMU_NO_CONTEXT)
^
219 _tlbil_va(vmaddr, pid, tsize, ind);
220 preempt_enable();
221 }
I don't know very much about PowerPC. The static checker is guessing
which headers to pull in instead of relying on the build system so there
are a lot of false positives. It's apparently using the
arch/powerpc/include/asm/book3s/64/tlbflush.h header which does:
#define MMU_NO_CONTEXT ~0UL
so it's UINT_MAX vs U64_MAX which is making the checker complain.
regards,
dan carpenter
[PATCH] powerpc/ps3: remove an unneeded NULL check
Static checkers don't like the inconsistent NULL checking on "ops".
This function is only called once and "ops" isn't NULL so the check can
be removed.
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
diff --git a/drivers/ps3/sys-manager-core.c b/drivers/ps3/sys-manager-core.c
index c429ffca1ab7..a5a6def77bb9 100644
--- a/drivers/ps3/sys-manager-core.c
+++ b/drivers/ps3/sys-manager-core.c
@@ -43,7 +43,7 @@ void ps3_sys_manager_register_ops(const struct
ps3_sys_manager_ops *ops)
{
BUG_ON(!ops);
BUG_ON(!ops->dev);
- ps3_sys_manager_ops = ops ? *ops : ps3_sys_manager_ops;
+ ps3_sys_manager_ops = *ops;
}
EXPORT_SYMBOL_GPL(ps3_sys_manager_register_ops);
powerpc/perf: Fix a sizeof() typo so we allocate less memory
We're allocating the size of the struct which is 32 bytes when we should
be allocating sizeof(void *) which is 4 or 8 bytes depending on the
architecture.
Fixes: 885dcd709ba9 ("powerpc/perf: Add nest IMC PMU support")
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
diff --git a/arch/powerpc/perf/imc-pmu.c b/arch/powerpc/perf/imc-pmu.c
index 88126245881b..91c90ddab807 100644
--- a/arch/powerpc/perf/imc-pmu.c
+++ b/arch/powerpc/perf/imc-pmu.c
@@ -210,7 +210,7 @@ static int update_events_in_group(struct device_node *node,
struct imc_pmu *pmu)
of_property_read_u32(node, "reg", _reg);
/* Allocate memory for the events */
- pmu->events = kcalloc(ct, sizeof(struct imc_events), GFP_KERNEL);
+ pmu->events = kcalloc(ct, sizeof(*pmu->events), GFP_KERNEL);
if (!pmu->events)
return -ENOMEM;
Re: [PATCH 3/4] char/tpm: Improve a size determination in nine functions
On Thu, Oct 19, 2017 at 04:58:23PM +, alexander.stef...@infineon.com wrote: > > On Tue, Oct 17, 2017 at 11:50:05AM +, alexander.stef...@infineon.com > > wrote: > > > > > Replace the specification of data structures by pointer dereferences > > > > > as the parameter for the operator "sizeof" to make the corresponding > > > > > size > > > > > determination a bit safer according to the Linux coding style > > > > > convention. > > > > > > > > > > > > This patch does one style in favor of the other. > > > > > > I actually prefer that style, so I'd welcome this change :) > > > > > > > At the end it's Jarkko's call, though I would NAK this as I think some > > > > one already told this to you for some other similar patch(es). > > > > > > > > > > > > I even would suggest to stop doing this noisy stuff, which keeps people > > > > busy for nothing. > > > > > > Cleaning up old code is also worth something, even if does not change > > > one bit in the assembly output in the end... > > > > > > Alexander > > > > Quite insignificant clean up it is that does more harm that gives any > > benefit as any new change adds debt to backporting. > > > > Anyway, this has been a useful patch set for me in the sense that I have > > clearer picture now on discarding/accepting commits. > > Indeed. I have now a better understanding for why some code looks as ugly as > it does. > These patches aren't a part of a sensible attempt to clean up the code. The first two patches are easy to review and have a clear benefit so that's fine any time. Patch 3 updates the code to a new style guideline but it doesn't really improve readability that much. Those sorts of patches are hard to review because you have to verify that the object code doesn't change. Plus it messes up git blame. It's good for new code and staging, but for old code, it's probably only worth it if there are other patches which make worth the price. You're basically applying it to make the patch sender happy. With patch 4, the tpm_ibmvtpm_probe() function was buggy and it's still buggy after the patch is applied. It's a waste of time re-arranging the code if you aren't going to fix the bugs. regards, dan carpenter
Re: [PATCH 4/4] char/tpm: Less checks in tpm_ibmvtpm_probe() after error detection
On Thu, Oct 19, 2017 at 04:16:37PM +0200, Michal Suchánek wrote:
> On Thu, 19 Oct 2017 16:36:46 +0300
> Dan Carpenter <dan.carpen...@oracle.com> wrote:
>
> > On Thu, Oct 19, 2017 at 01:56:32PM +0200, Michal Suchánek wrote:
> > >
> > > I think a single cleanup section is better than many labels that
> > > just avoid a single null check.
> > >
> >
> > I am not a big advocate of churn, but one err style error handling is
> > really bug prone.
> >
> > I'm dealing with static analysis so most of the bugs I see are error
> > handling bugs.
>
> But it looks like you do not deal much with actual code development
> because then you would know that some of the changes proposed by the
> static analysis lead to errors later when the code dynamically changes.
>
This is silly... Anyway, my record is there in git. I mostly send
bugfixes, and not cleanups. In terms of patches that are merged, I
probably have introduced one or two runtime bugs per year over the past
decade.
> > That's because error handling is hard to test but easy
> > for static analysis. One err style error handling is the worst
> > because you get things like:
> >
> > fail:
> > kfree(foo->bar);
> > kfree(foo);
> >
> > Oops, foo->bar is a NULL dereference. And generally, it's a bad thing
> > to free things that haven't been allocated so, for example, I see
> > refcounting bugs in error handling paths as well where we decrement
> > something that wasn't incremented. Freeing everything is more
> > complicated than just freeing one specific thing the way standard
> > kernel error handling works.
>
> It depends on the function in question. If it only allocates memory
> which is not reference-counted and kfree() checks for the null in most
> cases it is easier to do just one big cleanup.
>
No. Just always do it standard way. If there is only one error
condition just free it directly:
if (invalid) {
free(foo);
return -EINVAL;
}
But once you add a second error condition then use gotos. There is no
reason to deviate from the standard.
> If it is more complex more labels are preferable.
>
> >
> > > As long as you can tell easily which resources were already
> > > allocated and need to be freed it is saner to keep only one cleanup
> > > section.
> >
> > Sure, if the function is simple and short then the error handling is
> > normally simple and short. This is true for any style of error
> > handling.
> >
> > > If the code doing the allocation is changed in the future the single
> > > cleanup can stay whereas multiple labels have to be rewritten
> > > again.
> >
> > No, they don't unless you choose bad label names.
>
> You obviously miss the fact that resource allocation is not always
> added at the end of the function.
>
No, in my example, the new allocation was added to the *start* not the
end. It doesn't matter though, standard error handling works the same
either way.
> You may need to reorder the code and hence the order of allocation or
> add allocation at the start. Both of these cases break multiple labels
> and special cases but work with one big cleanup just fine.
No, You don't need to re-order the labels or change them. The label
name says what is freed. The order is the reverse order from how they
were allocated. It's very simple. You just have to keep track of the
most recently allocated thing:
foo = alloc();
if (!foo)
return -ENOMEM;
bar = alloc();
if (!bar)
goto free_foo;
baz = alloc();
if (!baz)
goto free_bar;
You can tell this code doesn't leak just from looking at the label
names.
regards,
dan carpenter
Re: [PATCH 4/4] char/tpm: Less checks in tpm_ibmvtpm_probe() after error detection
tpm_ibmvtpm_probe() is an example of poor names. It has the generic
ones like "cleanup" which don't say *what* is cleaned and the come-from
ones like "init_irq_cleanup" which don't say anything useful at all:
647 rc = request_irq(vio_dev->irq, ibmvtpm_interrupt, 0,
648 tpm_ibmvtpm_driver_name, ibmvtpm);
649 if (rc) {
650 dev_err(dev, "Error %d register irq 0x%x\n", rc,
vio_dev->irq);
651 goto init_irq_cleanup;
652 }
653
654 rc = vio_enable_interrupts(vio_dev);
655 if (rc) {
656 dev_err(dev, "Error %d enabling interrupts\n", rc);
657 goto init_irq_cleanup;
658 }
Sadly, we never do call free_irq().
> It can do it automatically, in most cases better, and automatically
> adapt it to code changes.
I've heard this before that if you only have one label that does
everything then it's "automatic" and "future proof". It's not true.
The best error handling is methodical error handling:
1) In the reverse order from how things were allocated
2) That mirrors the allocations exactly
3) That frees one thing at a time
4) With a proper, useful, readable label name which says what the goto
does
5) That doesn't free anything which hasn't been allocated
regards,
dan carpenter
Re: [PATCH 4/4] char/tpm: Less checks in tpm_ibmvtpm_probe() after error detection
On Thu, Oct 19, 2017 at 01:56:32PM +0200, Michal Suchánek wrote:
>
> I think a single cleanup section is better than many labels that just
> avoid a single null check.
>
I am not a big advocate of churn, but one err style error handling is
really bug prone.
I'm dealing with static analysis so most of the bugs I see are error
handling bugs. That's because error handling is hard to test but easy
for static analysis. One err style error handling is the worst because
you get things like:
fail:
kfree(foo->bar);
kfree(foo);
Oops, foo->bar is a NULL dereference. And generally, it's a bad thing
to free things that haven't been allocated so, for example, I see
refcounting bugs in error handling paths as well where we decrement
something that wasn't incremented. Freeing everything is more
complicated than just freeing one specific thing the way standard kernel
error handling works.
> As long as you can tell easily which resources were already allocated
> and need to be freed it is saner to keep only one cleanup section.
>
Sure, if the function is simple and short then the error handling is
normally simple and short. This is true for any style of error
handling.
> If the code doing the allocation is changed in the future the single
> cleanup can stay whereas multiple labels have to be rewritten again.
No, they don't unless you choose bad label names. Perhaps numbered
labels? We don't get a lot of those in the kernel any more. Label
name should be based on what the label does. Often I see bad label
names like generic labels:
foo = kmalloc();
if (!foo)
goto out;
What is out going to do? Another common anti-pattern is come-from
labels:
foo = kmalloc();
if (!foo)
goto kmalloc_failed;
Obviously, we can see from the if statement that the alloc failed and
you *just* know the next line is going to be is going to be:
if (invalid)
goto kmalloc_failed;
Which is wrong because kmalloc didn't fail... But if the label name is
based on what it does then, when you add or a remove an allocation, you
just have to edit the one thing. It's very simple:
+ foo = new_alloc();
+ if (!foo)
+ return -ENOMEM;
+
bar = old_alloc();
- if (!bar)
- return -ENOMEM;
+ if (!bar) {
+ ret -ENOMEM;
+ goto free_foo;
[ snip ]
free_whatever:
free(whatever);
+free_foo:
+ free(foo);
return ret;
>
> Also just changing this just for the sake of code style does not seem
> worth it whatever style you prefer.
True.
regards,
dan carpenter
Re: [PATCH 3/5] powerpc-pseries: Delete an unnecessary variable initialisation in iommu_pseries_alloc_group()
On Thu, Oct 19, 2017 at 01:37:18PM +0200, Michal Suchánek wrote:
> Hello,
>
> On Wed, 18 Oct 2017 21:24:25 +0200
> SF Markus Elfring <elfr...@users.sourceforge.net> wrote:
>
> > From: Markus Elfring <elfr...@users.sourceforge.net>
> > Date: Wed, 18 Oct 2017 19:14:39 +0200
> >
> > The variable "table_group" will be set to an appropriate pointer.
> > Thus omit the explicit initialisation at the beginning.
> >
> > Signed-off-by: Markus Elfring <elfr...@users.sourceforge.net>
> > ---
> > arch/powerpc/platforms/pseries/iommu.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/powerpc/platforms/pseries/iommu.c
> > b/arch/powerpc/platforms/pseries/iommu.c index
> > b37d4fb20d1c..b6c12b8e3ace 100644 ---
> > a/arch/powerpc/platforms/pseries/iommu.c +++
> > b/arch/powerpc/platforms/pseries/iommu.c @@ -55,7 +55,7 @@
> >
> > static struct iommu_table_group *iommu_pseries_alloc_group(int node)
> > {
> > - struct iommu_table_group *table_group = NULL;
> > + struct iommu_table_group *table_group;
> > struct iommu_table *tbl = NULL;
> > struct iommu_table_group_link *tgl = NULL;
> >
>
> I think initializing pointers to NULL is generally a good idea.
>
> If there is no use of the variable before it is reinitialized by
> allocation gcc is free to optimize out the variable and its initial
> value.
>
> On the other hand, if the code is changed later and use of the variable
> becomes possible you may crash (and get a gcc warning, too).
No, it's the opposite. GCC doesn't warn about potential NULL
dereferences, it warns about uninitialized variables. By initializing
it to a bogus value, you're deliberately disabling static analysis.
We do see bugs where, if only people didn't initialize stuff to bogus
values, then the bug would have been caught before it was merged.
You might imagine that static analysis tools would catch NULL
dereferences but it's actually really really hard. We used to have
an __uninitialized_var() macro which was used to silence GCC false
positives, but now we initialize the pointers to NULL instead. So
most of the code that you're dealing with is stuff that was marked as
too hard for GCC to understand. It's tricky.
regards,
dan carpenter
Re: [PATCH 0/4] char-TPM: Adjustments for ten function implementations
On Tue, Oct 17, 2017 at 10:56:42AM +0200, Julia Lawall wrote: > > > On Tue, 17 Oct 2017, Dan Carpenter wrote: > > > On Mon, Oct 16, 2017 at 09:35:12PM +0300, Jarkko Sakkinen wrote: > > > > > > A minor complaint: all commits are missing "Fixes:" tag. > > > > > > > Fixes is only for bug fixes. These don't fix any bugs. > > 0-day seems to put Fixes for everything. Should they be removed when the > old code is undesirable but doesn't actually cause a crash, eg out of date > API. Yeah, I feel like Fixes tags don't belong for API updates and cleanups. regards, dan carpenter
Re: [PATCH 0/4] char-TPM: Adjustments for ten function implementations
On Mon, Oct 16, 2017 at 09:35:12PM +0300, Jarkko Sakkinen wrote: > > A minor complaint: all commits are missing "Fixes:" tag. > Fixes is only for bug fixes. These don't fix any bugs. regards, dan carpenter
Re: [PATCH] powerpc/44x: mask and shift to zero bug
On Mon, Aug 28, 2017 at 10:57:05AM +0300, Dan Carpenter wrote: > I sent this email during kernel summit and neither of us could send a > patch at the time and we both problem forgot. I definitely forgot. > s/problem/probably/... I suck at email. :( regards, dan carpenter
Re: [PATCH] powerpc/44x: mask and shift to zero bug
On Sun, Aug 27, 2017 at 02:56:31PM +1000, Benjamin Herrenschmidt wrote:
> On Fri, 2017-08-25 at 13:33 +0300, Dan Carpenter wrote:
> > My static checker complains that 0x1800 >> 13 is zero. Looking at
> > the context, it seems like a copy and paste bug from the line below and
> > probably 0x3 << 13 or 0x6000 was intended.
> >
> > Fixes: 2af59f7d5c3e ("[POWERPC] 4xx: Add 405GPr and 405EP support in boot
> > wrapper")
> > Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
> > ---
> > Not tested!
> >
> > diff --git a/arch/powerpc/boot/4xx.c b/arch/powerpc/boot/4xx.c
> > index 9d3bd4c45a24..f7da65169124 100644
> > --- a/arch/powerpc/boot/4xx.c
> > +++ b/arch/powerpc/boot/4xx.c
> > @@ -564,7 +564,7 @@ void ibm405gp_fixup_clocks(unsigned int sys_clk,
> > unsigned int ser_clk)
> > fbdv = 16;
> > cbdv = ((pllmr & 0x0006) >> 17) + 1; /* CPU:PLB */
> > opdv = ((pllmr & 0x00018000) >> 15) + 1; /* PLB:OPB */
> > - ppdv = ((pllmr & 0x1800) >> 13) + 1; /* PLB:PCI */
> > + ppdv = ((pllmr & 0x6000) >> 13) + 1; /* PLB:PCI */
> > epdv = ((pllmr & 0x1800) >> 11) + 2; /* PLB:EBC */
> > udiv = ((cpc0_cr0 & 0x3e) >> 1) + 1;
>
> That rings a bell... Is this something we tried to fix before and had
> problems ? The thing is when I opened the 405GP and EP manual PDF,
> evince had memorized that this register was the last page I looked at
> :-) And I don't remember how many years ago that is.
>
> According to the 405gp spec ppdv is IBM bits 17,18 so your patch is
> correct.
>
> Acked-by: Benjamin Herrenschmidt <b...@kernel.crashing.org>
Hm... I did a search through my mailbox and you're right we discussed
this before.
[bug report] [POWERPC] 4xx: Add 405GPr and 405EP support in boot wrapper
I sent this email during kernel summit and neither of us could send a
patch at the time and we both problem forgot. I definitely forgot.
regards,
dan carpenter
[bug report] powerpc/mm/cxl: Add the fault handling cpu to mm cpumask
Hello Aneesh Kumar K.V,
This is a semi-automatic email about new static checker warnings.
The patch 0f4bc0932e51: "powerpc/mm/cxl: Add the fault handling cpu
to mm cpumask" from Jul 27, 2017, leads to the following Smatch
complaint:
drivers/misc/cxl/fault.c:161 cxl_handle_mm_fault()
warn: variable dereferenced before check 'mm' (see line 146)
drivers/misc/cxl/fault.c
145 */
146 cpumask_set_cpu(smp_processor_id(), mm_cpumask(mm));
^^
The patch adds an unchecked dereference.
147 if ((result = copro_handle_mm_fault(mm, dar, dsisr, ))) {
148 pr_devel("copro_handle_mm_fault failed: %#x\n", result);
149 return result;
150 }
151
152 if (!radix_enabled()) {
153 /*
154 * update_mmu_cache() will not have loaded the hash
since current->trap
155 * is not a 0x400 or 0x300, so just call hash_page_mm()
here.
156 */
157 access = _PAGE_PRESENT | _PAGE_READ;
158 if (dsisr & CXL_PSL_DSISR_An_S)
159 access |= _PAGE_WRITE;
160
161 if (!mm && (REGION_ID(dar) != USER_REGION_ID))
^^
But the existing code is careful to check "mm" for NULL. The
copro_handle_mm_fault() and hash_page_mm() both have checks built in.
162 access |= _PAGE_PRIVILEGED;
163
regards,
dan carpenter
[PATCH] powerpc/44x: mask and shift to zero bug
My static checker complains that 0x1800 >> 13 is zero. Looking at
the context, it seems like a copy and paste bug from the line below and
probably 0x3 << 13 or 0x6000 was intended.
Fixes: 2af59f7d5c3e ("[POWERPC] 4xx: Add 405GPr and 405EP support in boot
wrapper")
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
---
Not tested!
diff --git a/arch/powerpc/boot/4xx.c b/arch/powerpc/boot/4xx.c
index 9d3bd4c45a24..f7da65169124 100644
--- a/arch/powerpc/boot/4xx.c
+++ b/arch/powerpc/boot/4xx.c
@@ -564,7 +564,7 @@ void ibm405gp_fixup_clocks(unsigned int sys_clk, unsigned
int ser_clk)
fbdv = 16;
cbdv = ((pllmr & 0x0006) >> 17) + 1; /* CPU:PLB */
opdv = ((pllmr & 0x00018000) >> 15) + 1; /* PLB:OPB */
- ppdv = ((pllmr & 0x1800) >> 13) + 1; /* PLB:PCI */
+ ppdv = ((pllmr & 0x6000) >> 13) + 1; /* PLB:PCI */
epdv = ((pllmr & 0x1800) >> 11) + 2; /* PLB:EBC */
udiv = ((cpc0_cr0 & 0x3e) >> 1) + 1;
Re: [bug report] powerpc/mm/radix: Avoid flushing the PWC on every flush_tlb_range
On Fri, Aug 11, 2017 at 03:16:08PM -0700, Tyrel Datwyler wrote:
> On 08/11/2017 01:15 PM, Dan Carpenter wrote:
> > Hello Benjamin Herrenschmidt,
> >
> > This is a semi-automatic email about new static checker warnings.
> >
> > The patch 424de9c6e3f8: "powerpc/mm/radix: Avoid flushing the PWC on
> > every flush_tlb_range" from Jul 19, 2017, leads to the following
> > Smatch complaint:
> >
> > arch/powerpc/mm/tlb-radix.c:368 radix__flush_tlb_collapsed_pmd()
> > error: we previously assumed 'mm' could be null (see line 362)
> >
> > arch/powerpc/mm/tlb-radix.c
> >361
> >362 pid = mm ? mm->context.id : 0;
> > ^^
> > Check for NULL.
> >
> >363 if (unlikely(pid == MMU_NO_CONTEXT))
> >364 goto no_context;
> >365
> >366 /* 4k page size, just blow the world */
> >367 if (PAGE_SIZE == 0x1000) {
> >368 radix__flush_all_mm(mm);
> > ^^
> > Unchecked dereference.
>
> Appears to be a false positive. MMU_NO_CONTEXT I believe is defined as "0".
> So, maybe it
> would be clearer that we take the goto branch if this line read:
>
> 362 pid = mm ? mm->context.id : MMU_NO_CONTEXT;
>
Ah... This is because I'm compiling code for other arches in a "best
effort" way. It doesn't pull in the right headers so it doesn't know
the value of MMU_NO_CONTEXT. Otherwise it would read the code correctly
and not complain.
Sorry for that.
regards,
dan carpenter
[bug report] powerpc/mm/radix: Avoid flushing the PWC on every flush_tlb_range
Hello Benjamin Herrenschmidt,
This is a semi-automatic email about new static checker warnings.
The patch 424de9c6e3f8: "powerpc/mm/radix: Avoid flushing the PWC on
every flush_tlb_range" from Jul 19, 2017, leads to the following
Smatch complaint:
arch/powerpc/mm/tlb-radix.c:368 radix__flush_tlb_collapsed_pmd()
error: we previously assumed 'mm' could be null (see line 362)
arch/powerpc/mm/tlb-radix.c
361
362 pid = mm ? mm->context.id : 0;
^^
Check for NULL.
363 if (unlikely(pid == MMU_NO_CONTEXT))
364 goto no_context;
365
366 /* 4k page size, just blow the world */
367 if (PAGE_SIZE == 0x1000) {
368 radix__flush_all_mm(mm);
^^
Unchecked dereference.
369 return;
370 }
regards,
dan carpenter
