[Qemu-devel] [[RFC]PATCH:hw/sd:sd_init()] hw/sd : modified the sd_init() function

[Qemu-devel] [[RFC]PATCH:hw/sd:sd_init()] hw/sd : modified the sd_init() function

[Tejaswini]

From: Tejaswini Poluri <tejaswinipoluri3@gmail.com>

Changed sd_init() to take SDstate by value and return state as success/failure
Edited the rest of the functions using sd_init() to accommodate the change

Signed-off-by: Tejaswini Poluri <tejaswinipoluri3@gmail.com>
---
 hw/sd/milkymist-memcard.c |  3 +--
 hw/sd/omap_mmc.c          |  6 ++----
 hw/sd/pl181.c             |  4 ++--
 hw/sd/sd.c                | 13 ++++++++-----
 hw/sd/ssi-sd.c            |  3 +--
 include/hw/sd/sd.h        |  2 +-
 6 files changed, 15 insertions(+), 16 deletions(-)

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);
-- 
2.7.4

Re: [Qemu-devel] [[RFC]PATCH:hw/sd:sd_init()] hw/sd : modified the sd_init() function

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1219 bytes --]

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.

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [[RFC]PATCH:hw/sd:sd_init()] hw/sd : modified the sd_init() function

[Tejaswini Poluri]

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

Re: [Qemu-devel] [[RFC]PATCH:hw/sd:sd_init()] hw/sd : modified the sd_init() function

[Tejaswini Poluri]

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
>

Re: [Qemu-devel] [[RFC]PATCH:hw/sd:sd_init()] hw/sd : modified the sd_init() function

[Tejaswini Poluri]

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
>>
>


-- 
Regards,
Tejaswini