答复: [PATCH] bus: mhi: core: Fix null pointer access

答复: [PATCH] bus: mhi: core: Fix null pointer access

[Carl Yin(殷张成)]

Hi bbhatt:

On Wednesday, October 28, 2020 9:02 AM, bbhatt wrote:

> Hi Carl,
> 
> Yes this change is needed. Good catch. I ran in to this issue as well when a
> dev_err() call was made with a bad MHI configuration.
[carl.yin] yes, I also meet this error with a bad MHI configuration.
> 
> Maybe you can explain a little more in the commit text subject?
> 
> You could say, "Fix null pointer access when parsing MHI configuration"?
> 
> On 2020-10-26 22:33, carl.yin@quectel.com wrote:
> > From: carl <carl.yin@quectel.com>
> >
> > function parse_ev_cfg and parse_ch_cfg access mhi_cntrl->mhi_dev
> Functions parse_ev_cfg() and parse_ch_cfg()
> > before it is set in function mhi_register_controller,
> mhi_register_controller()
> > use cntrl_dev to instead mhi_dev.
> use cntrl_dev instead of mhi_dev.
> >
> > Signed-off-by: carl <carl.yin@quectel.com>
> With these commit text updates,
[carl.yin] thank for words correction, for my mother language is not English, 
there are lots of words wrong in the commit.
> 
> Reviewed-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
> > ---
> >  drivers/bus/mhi/core/init.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> > index 0ffdebde8..c6b43e90b 100644
> > --- a/drivers/bus/mhi/core/init.c
> > +++ b/drivers/bus/mhi/core/init.c
> > @@ -610,7 +610,7 @@ static int parse_ev_cfg(struct mhi_controller
> > *mhi_cntrl,  {
> >  	struct mhi_event *mhi_event;
> >  	const struct mhi_event_config *event_cfg;
> > -	struct device *dev = &mhi_cntrl->mhi_dev->dev;
> > +	struct device *dev = mhi_cntrl->cntrl_dev;
> >  	int i, num;
> >
> >  	num = config->num_events;
> > @@ -692,7 +692,7 @@ static int parse_ch_cfg(struct mhi_controller
> > *mhi_cntrl,
> >  			const struct mhi_controller_config *config)  {
> >  	const struct mhi_channel_config *ch_cfg;
> > -	struct device *dev = &mhi_cntrl->mhi_dev->dev;
> > +	struct device *dev = mhi_cntrl->cntrl_dev;
> >  	int i;
> >  	u32 chan;
> 
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a
> Linux Foundation Collaborative Project

Re: 答复: [PATCH] bus: mhi: core: Fix null pointer access

[Bhaumik Bhatt]

On 2020-10-27 19:03, Carl Yin wrote:
> Hi bbhatt:
> 
> On Wednesday, October 28, 2020 9:02 AM, bbhatt wrote:
> 
>> Hi Carl,
>> 
>> Yes this change is needed. Good catch. I ran in to this issue as well 
>> when a
>> dev_err() call was made with a bad MHI configuration.
> [carl.yin] yes, I also meet this error with a bad MHI configuration.
>> 
>> Maybe you can explain a little more in the commit text subject?
>> 
>> You could say, "Fix null pointer access when parsing MHI 
>> configuration"?
>> 
>> On 2020-10-26 22:33, carl.yin@quectel.com wrote:
>> > From: carl <carl.yin@quectel.com>
>> >
>> > function parse_ev_cfg and parse_ch_cfg access mhi_cntrl->mhi_dev
>> Functions parse_ev_cfg() and parse_ch_cfg()
>> > before it is set in function mhi_register_controller,
>> mhi_register_controller()
>> > use cntrl_dev to instead mhi_dev.
>> use cntrl_dev instead of mhi_dev.
>> >
>> > Signed-off-by: carl <carl.yin@quectel.com>
>> With these commit text updates,
> [carl.yin] thank for words correction, for my mother language is not 
> English,
> there are lots of words wrong in the commit.
>> 
No problem.
>> Reviewed-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
>> > ---
>> >  drivers/bus/mhi/core/init.c | 4 ++--
>> >  1 file changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
>> > index 0ffdebde8..c6b43e90b 100644
>> > --- a/drivers/bus/mhi/core/init.c
>> > +++ b/drivers/bus/mhi/core/init.c
>> > @@ -610,7 +610,7 @@ static int parse_ev_cfg(struct mhi_controller
>> > *mhi_cntrl,  {
>> >  	struct mhi_event *mhi_event;
>> >  	const struct mhi_event_config *event_cfg;
>> > -	struct device *dev = &mhi_cntrl->mhi_dev->dev;
>> > +	struct device *dev = mhi_cntrl->cntrl_dev;
>> >  	int i, num;
>> >
>> >  	num = config->num_events;
>> > @@ -692,7 +692,7 @@ static int parse_ch_cfg(struct mhi_controller
>> > *mhi_cntrl,
>> >  			const struct mhi_controller_config *config)  {
>> >  	const struct mhi_channel_config *ch_cfg;
>> > -	struct device *dev = &mhi_cntrl->mhi_dev->dev;
>> > +	struct device *dev = mhi_cntrl->cntrl_dev;
>> >  	int i;
>> >  	u32 chan;
>> 
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
>> Forum, a
>> Linux Foundation Collaborative Project

Can you also add a "Fixes:" tag to the patch?

Refer Documentation/process/submitting-patches.rst:

If your patch fixes a bug in a specific commit, e.g. you found an issue 
using
``git bisect``, please use the 'Fixes:' tag with the first 12 characters 
of
the SHA-1 ID, and the one line summary.  Do not split the tag across 
multiple
lines, tags are exempt from the "wrap at 75 columns" rule in order to 
simplify
parsing scripts.  For example::

	Fixes: 54a4f0239f2e ("KVM: MMU: make kvm_mmu_zap_page() return the 
number of pages it actually freed")

Thanks,
Bhaumik
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,
a Linux Foundation Collaborative Project