| Message ID | 1490610662-5483-1-git-send-email-tejaswinipoluri3@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Mar 27, 2017 at 4:43 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote: > On Mon, Mar 27, 2017 at 04:01:02PM +0530, Tejaswini wrote: > > From: Tejaswini Poluri <tejaswinipoluri3@gmail.com> > > Please shorten the subject line: "[PATCH] hw/sd: simplify sd_init() > prototype" > > > @@ -573,16 +573,19 @@ SDState *sd_init(BlockBackend *blk, bool is_spi) > > qdev_prop_set_drive(dev, "drive", blk, &err); > > if (err) { > > error_report("sd_init failed: %s", error_get_pretty(err)); > > - return NULL; > > + return -1; > > } > > qdev_prop_set_bit(dev, "spi", is_spi); > > object_property_set_bool(obj, true, "realized", &err); > > if (err) { > > error_report("sd_init failed: %s", error_get_pretty(err)); > > - return NULL; > > + return -1; > > } > > - > > - return SD_CARD(dev); > > + sd_state = SD_CARD(dev); > > The caller will not see the new value of sd_state. In C arguments are > passed by value. That means they are local variables inside the > function and do not affect the caller. > > The caller will call sd_init() along with passing SDState pointer as an argument like below if (sd_init(s->card, blk, false) < 0) . And the sd_init() function is modified to int sd_init(SDState *sd_state, BlockBackend *blk, bool is_spi) so that the caller gets the new value of SDstate updated. Looking forward for the comments of Paolo Bonzini to understand what more needs to be done as part of the task. I have CCed Paolo Bonzini, who posted this task. Maybe he can explain > what he meant by "Include SDState by value instead of allocating it in > sd_init (hw/sd/)". > > > + if (!sd_state) { > > + return -1; > > + } > > QEMU use 4 space indentation. Please do not use tabs. >
Hii Paolo, Waiting for your comments on this patch Regards, Tejaswini On 28 Mar 2017 11:01 a.m., "Tejaswini Poluri" <tejaswinipoluri3@gmail.com> wrote: > > > On Mon, Mar 27, 2017 at 4:43 PM, Stefan Hajnoczi <stefanha@redhat.com> > wrote: > >> On Mon, Mar 27, 2017 at 04:01:02PM +0530, Tejaswini wrote: >> > From: Tejaswini Poluri <tejaswinipoluri3@gmail.com> >> >> Please shorten the subject line: "[PATCH] hw/sd: simplify sd_init() >> prototype" >> >> > @@ -573,16 +573,19 @@ SDState *sd_init(BlockBackend *blk, bool is_spi) >> > qdev_prop_set_drive(dev, "drive", blk, &err); >> > if (err) { >> > error_report("sd_init failed: %s", error_get_pretty(err)); >> > - return NULL; >> > + return -1; >> > } >> > qdev_prop_set_bit(dev, "spi", is_spi); >> > object_property_set_bool(obj, true, "realized", &err); >> > if (err) { >> > error_report("sd_init failed: %s", error_get_pretty(err)); >> > - return NULL; >> > + return -1; >> > } >> > - >> > - return SD_CARD(dev); >> > + sd_state = SD_CARD(dev); >> >> The caller will not see the new value of sd_state. In C arguments are >> passed by value. That means they are local variables inside the >> function and do not affect the caller. >> >> The caller will call sd_init() along with passing SDState pointer as an > argument like below > if (sd_init(s->card, blk, false) < 0) . > And the sd_init() function is modified to > int sd_init(SDState *sd_state, BlockBackend *blk, bool is_spi) > so that the caller gets the new value of SDstate updated. > Looking forward for the comments of Paolo Bonzini to understand what more > needs to be done as part of the task. > > I have CCed Paolo Bonzini, who posted this task. Maybe he can explain >> what he meant by "Include SDState by value instead of allocating it in >> sd_init (hw/sd/)". >> >> > + if (!sd_state) { >> > + return -1; >> > + } >> >> QEMU use 4 space indentation. Please do not use tabs. >> > > > > -- > Regards, > Tejaswini >
Hii, A gentle reminder on the same! Regards, Tejaswini On Sun, Apr 9, 2017 at 5:44 AM, Tejaswini Poluri <tejaswinipoluri3@gmail.com > wrote: > Hii Paolo, > Waiting for your comments on this patch > > Regards, > Tejaswini > On 28 Mar 2017 11:01 a.m., "Tejaswini Poluri" <tejaswinipoluri3@gmail.com> > wrote: > >> >> >> On Mon, Mar 27, 2017 at 4:43 PM, Stefan Hajnoczi <stefanha@redhat.com> >> wrote: >> >>> On Mon, Mar 27, 2017 at 04:01:02PM +0530, Tejaswini wrote: >>> > From: Tejaswini Poluri <tejaswinipoluri3@gmail.com> >>> >>> Please shorten the subject line: "[PATCH] hw/sd: simplify sd_init() >>> prototype" >>> >>> > @@ -573,16 +573,19 @@ SDState *sd_init(BlockBackend *blk, bool is_spi) >>> > qdev_prop_set_drive(dev, "drive", blk, &err); >>> > if (err) { >>> > error_report("sd_init failed: %s", error_get_pretty(err)); >>> > - return NULL; >>> > + return -1; >>> > } >>> > qdev_prop_set_bit(dev, "spi", is_spi); >>> > object_property_set_bool(obj, true, "realized", &err); >>> > if (err) { >>> > error_report("sd_init failed: %s", error_get_pretty(err)); >>> > - return NULL; >>> > + return -1; >>> > } >>> > - >>> > - return SD_CARD(dev); >>> > + sd_state = SD_CARD(dev); >>> >>> The caller will not see the new value of sd_state. In C arguments are >>> passed by value. That means they are local variables inside the >>> function and do not affect the caller. >>> >>> The caller will call sd_init() along with passing SDState pointer as an >> argument like below >> if (sd_init(s->card, blk, false) < 0) . >> And the sd_init() function is modified to >> int sd_init(SDState *sd_state, BlockBackend *blk, bool is_spi) >> so that the caller gets the new value of SDstate updated. >> Looking forward for the comments of Paolo Bonzini to understand what more >> needs to be done as part of the task. >> >> I have CCed Paolo Bonzini, who posted this task. Maybe he can explain >>> what he meant by "Include SDState by value instead of allocating it in >>> sd_init (hw/sd/)". >>> >>> > + if (!sd_state) { >>> > + return -1; >>> > + } >>> >>> QEMU use 4 space indentation. Please do not use tabs. >>> >> >> >> >> -- >> Regards, >> Tejaswini >> >
diff --git a/hw/sd/milkymist-memcard.c b/hw/sd/milkymist-memcard.c index 1f2f0ed..68e50e5 100644 --- a/hw/sd/milkymist-memcard.c +++ b/hw/sd/milkymist-memcard.c @@ -259,8 +259,7 @@ static int milkymist_memcard_init(SysBusDevice *dev) /* FIXME use a qdev drive property instead of drive_get_next() */ dinfo = drive_get_next(IF_SD); blk = dinfo ? blk_by_legacy_dinfo(dinfo) : NULL; - s->card = sd_init(blk, false); - if (s->card == NULL) { + if (sd_init(s->card, blk, false) < 0) { return -1; } diff --git a/hw/sd/omap_mmc.c b/hw/sd/omap_mmc.c index e934cd3..add48a6 100644 --- a/hw/sd/omap_mmc.c +++ b/hw/sd/omap_mmc.c @@ -593,8 +593,7 @@ struct omap_mmc_s *omap_mmc_init(hwaddr base, memory_region_add_subregion(sysmem, base, &s->iomem); /* Instantiate the storage */ - s->card = sd_init(blk, false); - if (s->card == NULL) { + if (sd_init(s->card, blk, false) < 0) { exit(1); } @@ -620,8 +619,7 @@ struct omap_mmc_s *omap2_mmc_init(struct omap_target_agent_s *ta, omap_l4_attach(ta, 0, &s->iomem); /* Instantiate the storage */ - s->card = sd_init(blk, false); - if (s->card == NULL) { + if (sd_init(s->card, blk, false) < 0) { exit(1); } diff --git a/hw/sd/pl181.c b/hw/sd/pl181.c index 82c63a4..c2e2459 100644 --- a/hw/sd/pl181.c +++ b/hw/sd/pl181.c @@ -502,8 +502,8 @@ static void pl181_realize(DeviceState *dev, Error **errp) /* FIXME use a qdev drive property instead of drive_get_next() */ dinfo = drive_get_next(IF_SD); - s->card = sd_init(dinfo ? blk_by_legacy_dinfo(dinfo) : NULL, false); - if (s->card == NULL) { + if (sd_init(s->card, dinfo ? + blk_by_legacy_dinfo(dinfo) : NULL, false) < 0) { error_setg(errp, "sd_init failed"); } } diff --git a/hw/sd/sd.c b/hw/sd/sd.c index ba47bff..b593939 100644 --- a/hw/sd/sd.c +++ b/hw/sd/sd.c @@ -562,7 +562,7 @@ static const VMStateDescription sd_vmstate = { }; /* Legacy initialization function for use by non-qdevified callers */ -SDState *sd_init(BlockBackend *blk, bool is_spi) +int sd_init(SDState *sd_state, BlockBackend *blk, bool is_spi) { Object *obj; DeviceState *dev; @@ -573,16 +573,19 @@ SDState *sd_init(BlockBackend *blk, bool is_spi) qdev_prop_set_drive(dev, "drive", blk, &err); if (err) { error_report("sd_init failed: %s", error_get_pretty(err)); - return NULL; + return -1; } qdev_prop_set_bit(dev, "spi", is_spi); object_property_set_bool(obj, true, "realized", &err); if (err) { error_report("sd_init failed: %s", error_get_pretty(err)); - return NULL; + return -1; } - - return SD_CARD(dev); + sd_state = SD_CARD(dev); + if (!sd_state) { + return -1; + } + return 0; } void sd_set_cb(SDState *sd, qemu_irq readonly, qemu_irq insert) diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c index 24001dc..c70b32d 100644 --- a/hw/sd/ssi-sd.c +++ b/hw/sd/ssi-sd.c @@ -244,8 +244,7 @@ static void ssi_sd_realize(SSISlave *d, Error **errp) s->mode = SSI_SD_CMD; /* FIXME use a qdev drive property instead of drive_get_next() */ dinfo = drive_get_next(IF_SD); - s->sd = sd_init(dinfo ? blk_by_legacy_dinfo(dinfo) : NULL, true); - if (s->sd == NULL) { + if (sd_init(s->sd, dinfo ? blk_by_legacy_dinfo(dinfo) : NULL, true) < 0) { error_setg(errp, "Device initialization failed."); return; } diff --git a/include/hw/sd/sd.h b/include/hw/sd/sd.h index 96caefe..63741c0 100644 --- a/include/hw/sd/sd.h +++ b/include/hw/sd/sd.h @@ -115,7 +115,7 @@ typedef struct { } SDBusClass; /* Legacy functions to be used only by non-qdevified callers */ -SDState *sd_init(BlockBackend *bs, bool is_spi); +int sd_init(SDState *sd, BlockBackend *bs, bool is_spi); int sd_do_command(SDState *sd, SDRequest *req, uint8_t *response); void sd_write_data(SDState *sd, uint8_t value);