From: Dan Murphy <dmurphy@ti.com> To: Marc Kleine-Budde <mkl@pengutronix.de>, wg@grandegger.com, davem@davemloft.net Cc: linux-can@vger.kernel.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v11 1/5] can: m_can: Create a m_can platform framework Date: Wed, 8 May 2019 14:54:56 -0500 [thread overview] Message-ID: <35d179a7-2682-111e-638b-903559f0974a@ti.com> (raw) In-Reply-To: <8b53474d-9dbf-4b81-defd-1587e022990b@pengutronix.de> Marc Thanks for the comments On 5/8/19 9:35 AM, Marc Kleine-Budde wrote: > On 3/19/19 6:26 PM, Dan Murphy wrote: >> Create a m_can platform framework that peripheral >> devices can register to and use common code and register sets. >> The peripheral devices may provide read/write and configuration >> support of the IP. >> >> Acked-by: Wolfgang Grandegger <wg@grandegger.com> >> Signed-off-by: Dan Murphy <dmurphy@ti.com> > > [...] > >> -/* m_can private data structure */ >> -struct m_can_priv { >> - struct can_priv can; /* must be the first member */ >> - struct napi_struct napi; >> - struct net_device *dev; >> - struct device *device; >> - struct clk *hclk; >> - struct clk *cclk; >> - void __iomem *base; >> - u32 irqstatus; >> - int version; >> - >> - /* message ram configuration */ >> - void __iomem *mram_base; >> - struct mram_cfg mcfg[MRAM_CFG_NUM]; >> -}; >> +static u32 m_can_read(struct m_can_priv *priv, enum m_can_reg reg) >> +{ >> + if (priv->ops->read_reg) >> + return priv->ops->read_reg(priv, reg); >> + else >> + return -EINVAL; >> +} > > How do you plan to check the return value here? > What's the difference between a register value of 0xffffffe9 and > returning -EINVAL? > Good point. I could just inline this and return whatever is sent from the callback and as you said allow a backtrace to happen if read_reg is invalid. >> >> -static inline u32 m_can_read(const struct m_can_priv *priv, enum m_can_reg reg) >> +static int m_can_write(struct m_can_priv *priv, enum m_can_reg reg, u32 val) >> { >> - return readl(priv->base + reg); >> + if (priv->ops->write_reg) >> + return priv->ops->write_reg(priv, reg, val); >> + else >> + return -EINVAL; >> } > > I don't see anyone checking the return value. Better just dereference > the pointer and the kernel will produce a nice backtrace. > > Same should be done for all read and write variants. > I will need to go through this and see if there is any caller checking the return. But I think you are correct. If thats true I will just change this to a void, inline the function and allow a backtrace if the callback is null Dan > Marc >
WARNING: multiple messages have this Message-ID (diff)
From: Dan Murphy <dmurphy@ti.com> To: Marc Kleine-Budde <mkl@pengutronix.de>, <wg@grandegger.com>, <davem@davemloft.net> Cc: <linux-can@vger.kernel.org>, <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org> Subject: Re: [PATCH v11 1/5] can: m_can: Create a m_can platform framework Date: Wed, 8 May 2019 14:54:56 -0500 [thread overview] Message-ID: <35d179a7-2682-111e-638b-903559f0974a@ti.com> (raw) In-Reply-To: <8b53474d-9dbf-4b81-defd-1587e022990b@pengutronix.de> Marc Thanks for the comments On 5/8/19 9:35 AM, Marc Kleine-Budde wrote: > On 3/19/19 6:26 PM, Dan Murphy wrote: >> Create a m_can platform framework that peripheral >> devices can register to and use common code and register sets. >> The peripheral devices may provide read/write and configuration >> support of the IP. >> >> Acked-by: Wolfgang Grandegger <wg@grandegger.com> >> Signed-off-by: Dan Murphy <dmurphy@ti.com> > > [...] > >> -/* m_can private data structure */ >> -struct m_can_priv { >> - struct can_priv can; /* must be the first member */ >> - struct napi_struct napi; >> - struct net_device *dev; >> - struct device *device; >> - struct clk *hclk; >> - struct clk *cclk; >> - void __iomem *base; >> - u32 irqstatus; >> - int version; >> - >> - /* message ram configuration */ >> - void __iomem *mram_base; >> - struct mram_cfg mcfg[MRAM_CFG_NUM]; >> -}; >> +static u32 m_can_read(struct m_can_priv *priv, enum m_can_reg reg) >> +{ >> + if (priv->ops->read_reg) >> + return priv->ops->read_reg(priv, reg); >> + else >> + return -EINVAL; >> +} > > How do you plan to check the return value here? > What's the difference between a register value of 0xffffffe9 and > returning -EINVAL? > Good point. I could just inline this and return whatever is sent from the callback and as you said allow a backtrace to happen if read_reg is invalid. >> >> -static inline u32 m_can_read(const struct m_can_priv *priv, enum m_can_reg reg) >> +static int m_can_write(struct m_can_priv *priv, enum m_can_reg reg, u32 val) >> { >> - return readl(priv->base + reg); >> + if (priv->ops->write_reg) >> + return priv->ops->write_reg(priv, reg, val); >> + else >> + return -EINVAL; >> } > > I don't see anyone checking the return value. Better just dereference > the pointer and the kernel will produce a nice backtrace. > > Same should be done for all read and write variants. > I will need to go through this and see if there is any caller checking the return. But I think you are correct. If thats true I will just change this to a void, inline the function and allow a backtrace if the callback is null Dan > Marc >
next prev parent reply other threads:[~2019-05-08 19:54 UTC|newest] Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-03-19 17:26 [PATCH v11 1/5] can: m_can: Create a m_can platform framework Dan Murphy 2019-03-19 17:26 ` Dan Murphy 2019-03-19 17:26 ` [PATCH v11 2/5] can: m_can: Rename m_can_priv to m_can_classdev Dan Murphy 2019-03-19 17:26 ` Dan Murphy 2019-03-19 17:26 ` [PATCH v11 3/5] dt-bindings: can: tcan4x5x: Add DT bindings for TCAN4x5X driver Dan Murphy 2019-03-19 17:26 ` Dan Murphy 2019-03-19 17:26 ` [PATCH v11 4/5] can: tcan4x5x: Add tcan4x5x driver to the kernel Dan Murphy 2019-03-19 17:26 ` Dan Murphy 2019-03-19 17:26 ` [PATCH v11 5/5] can: m_can: Fix checkpatch issues on existing code Dan Murphy 2019-03-19 17:26 ` Dan Murphy 2019-04-02 12:03 ` [PATCH v11 1/5] can: m_can: Create a m_can platform framework Dan Murphy 2019-04-02 12:03 ` Dan Murphy 2019-04-02 12:17 ` Wolfgang Grandegger 2019-04-10 13:24 ` Dan Murphy 2019-04-10 13:24 ` Dan Murphy 2019-04-18 14:36 ` Dan Murphy 2019-04-18 14:36 ` Dan Murphy 2019-05-01 14:10 ` Dan Murphy 2019-05-01 14:10 ` Dan Murphy 2019-05-08 14:35 ` Marc Kleine-Budde 2019-05-08 19:54 ` Dan Murphy [this message] 2019-05-08 19:54 ` Dan Murphy 2019-05-09 6:38 ` Marc Kleine-Budde
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=35d179a7-2682-111e-638b-903559f0974a@ti.com \ --to=dmurphy@ti.com \ --cc=davem@davemloft.net \ --cc=linux-can@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mkl@pengutronix.de \ --cc=netdev@vger.kernel.org \ --cc=wg@grandegger.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.