| Message ID | 20181207152917.4862-9-georgi.djakov@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce on-chip interconnect API | expand |
Hi Georgi, On Friday 07 Dec 2018 at 17:29:17 (+0200), Georgi Djakov wrote: > Recently the cmd_db_read_aux_data() function was changed to avoid using > memcpy and return a pointer instead. Update the code to the new API and > fix the build failure. > > Fixes: ed3cafa79ea7 ("soc: qcom: cmd-db: Stop memcpy()ing in cmd_db_read_aux_data()") > Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org> > --- > drivers/interconnect/qcom/sdm845.c | 26 ++++++++++++++------------ IIUC this file is introduced by patch 5. Should the fix be squashed into patch 5 directly ? Just to keep things bisectable. Thanks, Quentin
Hi Quentin, On 12/7/18 18:27, Quentin Perret wrote: > Hi Georgi, > > On Friday 07 Dec 2018 at 17:29:17 (+0200), Georgi Djakov wrote: >> Recently the cmd_db_read_aux_data() function was changed to avoid using >> memcpy and return a pointer instead. Update the code to the new API and >> fix the build failure. >> >> Fixes: ed3cafa79ea7 ("soc: qcom: cmd-db: Stop memcpy()ing in cmd_db_read_aux_data()") >> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org> >> --- >> drivers/interconnect/qcom/sdm845.c | 26 ++++++++++++++------------ > > IIUC this file is introduced by patch 5. Should the fix be squashed > into patch 5 directly ? Just to keep things bisectable. The reason why i have split it as a separate change is because as a separate change it would be easier to review & test for the people who are already familiar with the rest of the series. Another minor reason is that a separate patch will also make the life a bit easier for some people who are back-porting this to kernels using the older version of the cmd_db API. The commit that changed the cmd_db API is not yet in mainline, but in linux-next. I am not sure what is preferred in this case? Thanks, Georgi
On Friday 07 Dec 2018 at 18:47:22 (+0200), Georgi Djakov wrote: > Hi Quentin, > > On 12/7/18 18:27, Quentin Perret wrote: > > Hi Georgi, > > > > On Friday 07 Dec 2018 at 17:29:17 (+0200), Georgi Djakov wrote: > >> Recently the cmd_db_read_aux_data() function was changed to avoid using > >> memcpy and return a pointer instead. Update the code to the new API and > >> fix the build failure. > >> > >> Fixes: ed3cafa79ea7 ("soc: qcom: cmd-db: Stop memcpy()ing in cmd_db_read_aux_data()") > >> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org> > >> --- > >> drivers/interconnect/qcom/sdm845.c | 26 ++++++++++++++------------ > > > > IIUC this file is introduced by patch 5. Should the fix be squashed > > into patch 5 directly ? Just to keep things bisectable. > > The reason why i have split it as a separate change is because as a > separate change it would be easier to review & test for the people who > are already familiar with the rest of the series. > > Another minor reason is that a separate patch will also make the life a > bit easier for some people who are back-porting this to kernels using > the older version of the cmd_db API. > > The commit that changed the cmd_db API is not yet in mainline, but in > linux-next. I am not sure what is preferred in this case? Not sure either but I guess that will depend who gets merged first ... If that's the cmd_db change, then you'll need to squash your fix in patch 5. If your series goes first, then the fix needs to be applied to the cmb_db change. I personally don't mind either way as long as we don't break bisection :-) Thanks, Quentin
Hi, On Fri, Dec 7, 2018 at 9:14 AM Quentin Perret <quentin.perret@arm.com> wrote: > > On Friday 07 Dec 2018 at 18:47:22 (+0200), Georgi Djakov wrote: > > Hi Quentin, > > > > On 12/7/18 18:27, Quentin Perret wrote: > > > Hi Georgi, > > > > > > On Friday 07 Dec 2018 at 17:29:17 (+0200), Georgi Djakov wrote: > > >> Recently the cmd_db_read_aux_data() function was changed to avoid using > > >> memcpy and return a pointer instead. Update the code to the new API and > > >> fix the build failure. > > >> > > >> Fixes: ed3cafa79ea7 ("soc: qcom: cmd-db: Stop memcpy()ing in cmd_db_read_aux_data()") > > >> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org> > > >> --- > > >> drivers/interconnect/qcom/sdm845.c | 26 ++++++++++++++------------ > > > > > > IIUC this file is introduced by patch 5. Should the fix be squashed > > > into patch 5 directly ? Just to keep things bisectable. > > > > The reason why i have split it as a separate change is because as a > > separate change it would be easier to review & test for the people who > > are already familiar with the rest of the series. > > > > Another minor reason is that a separate patch will also make the life a > > bit easier for some people who are back-porting this to kernels using > > the older version of the cmd_db API. > > > > The commit that changed the cmd_db API is not yet in mainline, but in > > linux-next. I am not sure what is preferred in this case? > > Not sure either but I guess that will depend who gets merged first ... > If that's the cmd_db change, then you'll need to squash your fix in > patch 5. If your series goes first, then the fix needs to be applied to > the cmb_db change. > > I personally don't mind either way as long as we don't break bisection :-) My $0.02 is that anyone downstream picking this will also be picking the cmd db change, so I'd suggest squashing as suggested by Quentin. Maybe you can do that and post a v12? Since it sounded like Greg was about ready to land [1] maybe we shouldn't wait too long till doing it? ...unfortunately I guess we need to figure out how this will actually land. Greg can't pick the series up directly since it won't compile without the cmd-db change. ...so we need to do one of the following: 1. Feed it through a tree that goes through arm-soc (Andy's tree or arm-soc directly) so you'll have commit ed3cafa79ea7 ("soc: qcom: cmd-db: Stop memcpy()ing in cmd_db_read_aux_data()"). 2. Greg could also merge in Andy's pull request (https://patchwork.kernel.org/patch/10710447/), but you'll get some unrelated patches too. 3. Wait until next year and we have the next -rc1 to get this merged. ...it seems like #1 would be the least painful option, but obviously others would have to be OK w/ it. [1] https://lkml.kernel.org/r/20181206145547.GA7884@kroah.com -Doug
diff --git a/drivers/interconnect/qcom/sdm845.c b/drivers/interconnect/qcom/sdm845.c index f594dd6f500f..06dbc99766ee 100644 --- a/drivers/interconnect/qcom/sdm845.c +++ b/drivers/interconnect/qcom/sdm845.c @@ -487,7 +487,9 @@ static struct qcom_icc_desc sdm845_rsc_hlos = { static int qcom_icc_bcm_init(struct qcom_icc_bcm *bcm, struct device *dev) { struct qcom_icc_node *qn; - int ret, i; + const struct bcm_db *data; + size_t data_count; + int i; bcm->addr = cmd_db_read_addr(bcm->name); if (!bcm->addr) { @@ -496,22 +498,22 @@ static int qcom_icc_bcm_init(struct qcom_icc_bcm *bcm, struct device *dev) return -EINVAL; } - if (cmd_db_read_aux_data_len(bcm->name) < sizeof(struct bcm_db)) { + data = cmd_db_read_aux_data(bcm->name, &data_count); + if (IS_ERR(data)) { + dev_err(dev, "%s command db read error (%ld)\n", + bcm->name, PTR_ERR(data)); + return PTR_ERR(data); + } + if (!data_count) { dev_err(dev, "%s command db missing or partial aux data\n", bcm->name); return -EINVAL; } - ret = cmd_db_read_aux_data(bcm->name, (u8 *)&bcm->aux_data, - sizeof(struct bcm_db)); - if (ret < 0) { - dev_err(dev, "%s command db read error (%d)\n", - bcm->name, ret); - return ret; - } - - bcm->aux_data.unit = le32_to_cpu(bcm->aux_data.unit); - bcm->aux_data.width = le16_to_cpu(bcm->aux_data.width); + bcm->aux_data.unit = le32_to_cpu(data->unit); + bcm->aux_data.width = le16_to_cpu(data->width); + bcm->aux_data.vcd = data->vcd; + bcm->aux_data.reserved = data->reserved; /* * Link Qnodes to their respective BCMs
Recently the cmd_db_read_aux_data() function was changed to avoid using memcpy and return a pointer instead. Update the code to the new API and fix the build failure. Fixes: ed3cafa79ea7 ("soc: qcom: cmd-db: Stop memcpy()ing in cmd_db_read_aux_data()") Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org> --- drivers/interconnect/qcom/sdm845.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-)